Re: [PATCH 2/5] hw_random: use reference counts on each struct hwrng.
On Thu, Sep 18, 2014 at 12:18:23PM +0930, Rusty Russell wrote: > current_rng holds one reference, and we bump it every time we want > to do a read from it. > > This means we only hold the rng_mutex to grab or drop a reference, > so accessing /sys/devices/virtual/misc/hw_random/rng_current doesn't > block on read of /dev/hwrng. > > Using a kref is overkill (we're always under the rng_mutex), but > a standard pattern. > > This also solves the problem that the hwrng_fillfn thread was > accessing current_rng without a lock, which could change (eg. to NULL) > underneath it. Hi Rusty, > Signed-off-by: Rusty Russell > --- > drivers/char/hw_random/core.c | 135 > -- > include/linux/hw_random.h | 2 + > 2 files changed, 94 insertions(+), 43 deletions(-) ... > static int rng_dev_open(struct inode *inode, struct file *filp) > { > /* enforce read-only access to this chrdev */ > @@ -154,21 +202,22 @@ static ssize_t rng_dev_read(struct file *filp, char > __user *buf, > ssize_t ret = 0; > int err = 0; > int bytes_read, len; > + struct hwrng *rng; > > while (size) { > - if (mutex_lock_interruptible(_mutex)) { > - err = -ERESTARTSYS; > + rng = get_current_rng(); > + if (IS_ERR(rng)) { > + err = PTR_ERR(rng); > goto out; > } > - > - if (!current_rng) { > + if (!rng) { > err = -ENODEV; > - goto out_unlock; > + goto out; > } > > mutex_lock(_mutex); > if (!data_avail) { > - bytes_read = rng_get_data(current_rng, rng_buffer, > + bytes_read = rng_get_data(rng, rng_buffer, > rng_buffer_size(), > !(filp->f_flags & O_NONBLOCK)); > if (bytes_read < 0) { > @@ -200,7 +249,6 @@ static ssize_t rng_dev_read(struct file *filp, char > __user *buf, > ret += len; > } > > - mutex_unlock(_mutex); > mutex_unlock(_mutex); > > if (need_resched()) > @@ -210,15 +258,16 @@ static ssize_t rng_dev_read(struct file *filp, char > __user *buf, > err = -ERESTARTSYS; We need put_rng() in this error path. Otherwise, unhotplug will hang in the end of hwrng_unregister() |/* Just in case rng is reading right now, wait. */ |wait_event(rng_done, atomic_read(>ref.refcount) == 0); Steps to reproduce the hang: guest) # dd if=/dev/hwrng of=/dev/null cancel dd process after 10 seconds guest) # dd if=/dev/hwrng of=/dev/null & hotunplug rng device from qemu monitor result: device can't be removed (still can find in QEMU monitor) diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c index 96fa067..4e22d70 100644 --- a/drivers/char/hw_random/core.c +++ b/drivers/char/hw_random/core.c @@ -258,6 +258,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, if (signal_pending(current)) { err = -ERESTARTSYS; + put_rng(rng); goto out; } > goto out; > } > + > + put_rng(rng); > } > out: > return ret ? : err; > -out_unlock: > - mutex_unlock(_mutex); > - goto out; > + > out_unlock_reading: > mutex_unlock(_mutex); > - goto out_unlock; > + put_rng(rng); > + goto out; > } > > > @@ -257,8 +306,8 @@ static ssize_t hwrng_attr_current_store(struct device > *dev, > err = hwrng_init(rng); > if (err) > break; > - hwrng_cleanup(current_rng); > - current_rng = rng; > + drop_current_rng(); > + set_current_rng(rng); > err = 0; > break; > } > @@ -272,17 +321,15 @@ static ssize_t hwrng_attr_current_show(struct device > *dev, > struct device_attribute *attr, > char *buf) > { > - int err; > ssize_t ret; > - const char *name = "none"; > + struct hwrng *rng; > > - err = mutex_lock_interruptible(_mutex); > - if (err) > - return -ERESTARTSYS; > - if (current_rng) > - name = current_rng->name; > - ret = snprintf(buf, PAGE_SIZE, "%s\n", name); > - mutex_unlock(_mutex); > + rng = get_current_rng(); > + if (IS_ERR(rng)) > + return PTR_ERR(rng); > + > + ret = snprintf(buf, PAGE_SIZE, "%s\n", rng ? rng->name : "none"); > + put_rng(rng); > > return ret; > } > @@ -357,12 +404,16 @@ static int hwrng_fillfn(void
Re: [PATCH 2/5] hw_random: use reference counts on each struct hwrng.
On Thu, Sep 18, 2014 at 12:18:23PM +0930, Rusty Russell wrote: current_rng holds one reference, and we bump it every time we want to do a read from it. This means we only hold the rng_mutex to grab or drop a reference, so accessing /sys/devices/virtual/misc/hw_random/rng_current doesn't block on read of /dev/hwrng. Using a kref is overkill (we're always under the rng_mutex), but a standard pattern. This also solves the problem that the hwrng_fillfn thread was accessing current_rng without a lock, which could change (eg. to NULL) underneath it. Hi Rusty, Signed-off-by: Rusty Russell ru...@rustcorp.com.au --- drivers/char/hw_random/core.c | 135 -- include/linux/hw_random.h | 2 + 2 files changed, 94 insertions(+), 43 deletions(-) ... static int rng_dev_open(struct inode *inode, struct file *filp) { /* enforce read-only access to this chrdev */ @@ -154,21 +202,22 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, ssize_t ret = 0; int err = 0; int bytes_read, len; + struct hwrng *rng; while (size) { - if (mutex_lock_interruptible(rng_mutex)) { - err = -ERESTARTSYS; + rng = get_current_rng(); + if (IS_ERR(rng)) { + err = PTR_ERR(rng); goto out; } - - if (!current_rng) { + if (!rng) { err = -ENODEV; - goto out_unlock; + goto out; } mutex_lock(reading_mutex); if (!data_avail) { - bytes_read = rng_get_data(current_rng, rng_buffer, + bytes_read = rng_get_data(rng, rng_buffer, rng_buffer_size(), !(filp-f_flags O_NONBLOCK)); if (bytes_read 0) { @@ -200,7 +249,6 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, ret += len; } - mutex_unlock(rng_mutex); mutex_unlock(reading_mutex); if (need_resched()) @@ -210,15 +258,16 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, err = -ERESTARTSYS; We need put_rng() in this error path. Otherwise, unhotplug will hang in the end of hwrng_unregister() |/* Just in case rng is reading right now, wait. */ |wait_event(rng_done, atomic_read(rng-ref.refcount) == 0); Steps to reproduce the hang: guest) # dd if=/dev/hwrng of=/dev/null cancel dd process after 10 seconds guest) # dd if=/dev/hwrng of=/dev/null hotunplug rng device from qemu monitor result: device can't be removed (still can find in QEMU monitor) diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c index 96fa067..4e22d70 100644 --- a/drivers/char/hw_random/core.c +++ b/drivers/char/hw_random/core.c @@ -258,6 +258,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, if (signal_pending(current)) { err = -ERESTARTSYS; + put_rng(rng); goto out; } goto out; } + + put_rng(rng); } out: return ret ? : err; -out_unlock: - mutex_unlock(rng_mutex); - goto out; + out_unlock_reading: mutex_unlock(reading_mutex); - goto out_unlock; + put_rng(rng); + goto out; } @@ -257,8 +306,8 @@ static ssize_t hwrng_attr_current_store(struct device *dev, err = hwrng_init(rng); if (err) break; - hwrng_cleanup(current_rng); - current_rng = rng; + drop_current_rng(); + set_current_rng(rng); err = 0; break; } @@ -272,17 +321,15 @@ static ssize_t hwrng_attr_current_show(struct device *dev, struct device_attribute *attr, char *buf) { - int err; ssize_t ret; - const char *name = none; + struct hwrng *rng; - err = mutex_lock_interruptible(rng_mutex); - if (err) - return -ERESTARTSYS; - if (current_rng) - name = current_rng-name; - ret = snprintf(buf, PAGE_SIZE, %s\n, name); - mutex_unlock(rng_mutex); + rng = get_current_rng(); + if (IS_ERR(rng)) + return PTR_ERR(rng); + + ret = snprintf(buf, PAGE_SIZE, %s\n, rng ? rng-name : none); + put_rng(rng); return ret; } @@ -357,12 +404,16 @@ static int hwrng_fillfn(void *unused) long rc; while (!kthread_should_stop()) { -
[PATCH 2/5] hw_random: use reference counts on each struct hwrng.
current_rng holds one reference, and we bump it every time we want to do a read from it. This means we only hold the rng_mutex to grab or drop a reference, so accessing /sys/devices/virtual/misc/hw_random/rng_current doesn't block on read of /dev/hwrng. Using a kref is overkill (we're always under the rng_mutex), but a standard pattern. This also solves the problem that the hwrng_fillfn thread was accessing current_rng without a lock, which could change (eg. to NULL) underneath it. Signed-off-by: Rusty Russell --- drivers/char/hw_random/core.c | 135 -- include/linux/hw_random.h | 2 + 2 files changed, 94 insertions(+), 43 deletions(-) diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c index b1b6042ad85c..dc9092a1075d 100644 --- a/drivers/char/hw_random/core.c +++ b/drivers/char/hw_random/core.c @@ -42,6 +42,7 @@ #include #include #include +#include #include @@ -91,6 +92,59 @@ static void add_early_randomness(struct hwrng *rng) add_device_randomness(bytes, bytes_read); } +static inline void cleanup_rng(struct kref *kref) +{ + struct hwrng *rng = container_of(kref, struct hwrng, ref); + + if (rng->cleanup) + rng->cleanup(rng); +} + +static void set_current_rng(struct hwrng *rng) +{ + BUG_ON(!mutex_is_locked(_mutex)); + kref_get(>ref); + current_rng = rng; +} + +static void drop_current_rng(void) +{ + BUG_ON(!mutex_is_locked(_mutex)); + if (!current_rng) + return; + + kref_put(_rng->ref, cleanup_rng); + current_rng = NULL; +} + +/* Returns ERR_PTR(), NULL or refcounted hwrng */ +static struct hwrng *get_current_rng(void) +{ + struct hwrng *rng; + + if (mutex_lock_interruptible(_mutex)) + return ERR_PTR(-ERESTARTSYS); + + rng = current_rng; + if (rng) + kref_get(>ref); + + mutex_unlock(_mutex); + return rng; +} + +static void put_rng(struct hwrng *rng) +{ + /* +* Hold rng_mutex here so we serialize in case they set_current_rng +* on rng again immediately. +*/ + mutex_lock(_mutex); + if (rng) + kref_put(>ref, cleanup_rng); + mutex_unlock(_mutex); +} + static inline int hwrng_init(struct hwrng *rng) { if (rng->init) { @@ -113,12 +167,6 @@ static inline int hwrng_init(struct hwrng *rng) return 0; } -static inline void hwrng_cleanup(struct hwrng *rng) -{ - if (rng && rng->cleanup) - rng->cleanup(rng); -} - static int rng_dev_open(struct inode *inode, struct file *filp) { /* enforce read-only access to this chrdev */ @@ -154,21 +202,22 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, ssize_t ret = 0; int err = 0; int bytes_read, len; + struct hwrng *rng; while (size) { - if (mutex_lock_interruptible(_mutex)) { - err = -ERESTARTSYS; + rng = get_current_rng(); + if (IS_ERR(rng)) { + err = PTR_ERR(rng); goto out; } - - if (!current_rng) { + if (!rng) { err = -ENODEV; - goto out_unlock; + goto out; } mutex_lock(_mutex); if (!data_avail) { - bytes_read = rng_get_data(current_rng, rng_buffer, + bytes_read = rng_get_data(rng, rng_buffer, rng_buffer_size(), !(filp->f_flags & O_NONBLOCK)); if (bytes_read < 0) { @@ -200,7 +249,6 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, ret += len; } - mutex_unlock(_mutex); mutex_unlock(_mutex); if (need_resched()) @@ -210,15 +258,16 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, err = -ERESTARTSYS; goto out; } + + put_rng(rng); } out: return ret ? : err; -out_unlock: - mutex_unlock(_mutex); - goto out; + out_unlock_reading: mutex_unlock(_mutex); - goto out_unlock; + put_rng(rng); + goto out; } @@ -257,8 +306,8 @@ static ssize_t hwrng_attr_current_store(struct device *dev, err = hwrng_init(rng); if (err) break; - hwrng_cleanup(current_rng); - current_rng = rng; + drop_current_rng(); + set_current_rng(rng); err = 0; break; } @@ -272,17 +321,15 @@ static ssize_t
[PATCH 2/5] hw_random: use reference counts on each struct hwrng.
current_rng holds one reference, and we bump it every time we want to do a read from it. This means we only hold the rng_mutex to grab or drop a reference, so accessing /sys/devices/virtual/misc/hw_random/rng_current doesn't block on read of /dev/hwrng. Using a kref is overkill (we're always under the rng_mutex), but a standard pattern. This also solves the problem that the hwrng_fillfn thread was accessing current_rng without a lock, which could change (eg. to NULL) underneath it. Signed-off-by: Rusty Russell ru...@rustcorp.com.au --- drivers/char/hw_random/core.c | 135 -- include/linux/hw_random.h | 2 + 2 files changed, 94 insertions(+), 43 deletions(-) diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c index b1b6042ad85c..dc9092a1075d 100644 --- a/drivers/char/hw_random/core.c +++ b/drivers/char/hw_random/core.c @@ -42,6 +42,7 @@ #include linux/delay.h #include linux/slab.h #include linux/random.h +#include linux/err.h #include asm/uaccess.h @@ -91,6 +92,59 @@ static void add_early_randomness(struct hwrng *rng) add_device_randomness(bytes, bytes_read); } +static inline void cleanup_rng(struct kref *kref) +{ + struct hwrng *rng = container_of(kref, struct hwrng, ref); + + if (rng-cleanup) + rng-cleanup(rng); +} + +static void set_current_rng(struct hwrng *rng) +{ + BUG_ON(!mutex_is_locked(rng_mutex)); + kref_get(rng-ref); + current_rng = rng; +} + +static void drop_current_rng(void) +{ + BUG_ON(!mutex_is_locked(rng_mutex)); + if (!current_rng) + return; + + kref_put(current_rng-ref, cleanup_rng); + current_rng = NULL; +} + +/* Returns ERR_PTR(), NULL or refcounted hwrng */ +static struct hwrng *get_current_rng(void) +{ + struct hwrng *rng; + + if (mutex_lock_interruptible(rng_mutex)) + return ERR_PTR(-ERESTARTSYS); + + rng = current_rng; + if (rng) + kref_get(rng-ref); + + mutex_unlock(rng_mutex); + return rng; +} + +static void put_rng(struct hwrng *rng) +{ + /* +* Hold rng_mutex here so we serialize in case they set_current_rng +* on rng again immediately. +*/ + mutex_lock(rng_mutex); + if (rng) + kref_put(rng-ref, cleanup_rng); + mutex_unlock(rng_mutex); +} + static inline int hwrng_init(struct hwrng *rng) { if (rng-init) { @@ -113,12 +167,6 @@ static inline int hwrng_init(struct hwrng *rng) return 0; } -static inline void hwrng_cleanup(struct hwrng *rng) -{ - if (rng rng-cleanup) - rng-cleanup(rng); -} - static int rng_dev_open(struct inode *inode, struct file *filp) { /* enforce read-only access to this chrdev */ @@ -154,21 +202,22 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, ssize_t ret = 0; int err = 0; int bytes_read, len; + struct hwrng *rng; while (size) { - if (mutex_lock_interruptible(rng_mutex)) { - err = -ERESTARTSYS; + rng = get_current_rng(); + if (IS_ERR(rng)) { + err = PTR_ERR(rng); goto out; } - - if (!current_rng) { + if (!rng) { err = -ENODEV; - goto out_unlock; + goto out; } mutex_lock(reading_mutex); if (!data_avail) { - bytes_read = rng_get_data(current_rng, rng_buffer, + bytes_read = rng_get_data(rng, rng_buffer, rng_buffer_size(), !(filp-f_flags O_NONBLOCK)); if (bytes_read 0) { @@ -200,7 +249,6 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, ret += len; } - mutex_unlock(rng_mutex); mutex_unlock(reading_mutex); if (need_resched()) @@ -210,15 +258,16 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf, err = -ERESTARTSYS; goto out; } + + put_rng(rng); } out: return ret ? : err; -out_unlock: - mutex_unlock(rng_mutex); - goto out; + out_unlock_reading: mutex_unlock(reading_mutex); - goto out_unlock; + put_rng(rng); + goto out; } @@ -257,8 +306,8 @@ static ssize_t hwrng_attr_current_store(struct device *dev, err = hwrng_init(rng); if (err) break; - hwrng_cleanup(current_rng); - current_rng = rng; + drop_current_rng(); +