Re: [PATCH v2] Documentation:Chinese translation of Documentation/arm64/legacy_instructions.txt
On Thu, Mar 26, 2015 at 05:02:57PM +0800, w...@redhat.com wrote:
> From: Fu Wei
>
> This is a Chinese translated version of
> Documentation/arm64/legacy_instructions.txt
>
> It is based on the modifications of
> Documentation/arm64/legacy_instructions.txt in submission:
> "587064b6", "bd35a4ad", "2d888f48", "c852f320".
>
> Signed-off-by: Fu Wei
Reviewed-by: Amos Kong
> ---
> Documentation/zh_CN/arm64/legacy_instructions.txt | 72
> +++
> 1 file changed, 72 insertions(+)
>
> diff --git a/Documentation/zh_CN/arm64/legacy_instructions.txt
> b/Documentation/zh_CN/arm64/legacy_instructions.txt
> new file mode 100644
> index 000..68362a1
> --- /dev/null
> +++ b/Documentation/zh_CN/arm64/legacy_instructions.txt
> @@ -0,0 +1,72 @@
> +Chinese translated version of Documentation/arm64/legacy_instructions.txt
> +
> +If you have any comment or update to the content, please contact the
> +original document maintainer directly. However, if you have a problem
> +communicating in English you can also ask the Chinese maintainer for
> +help. Contact the Chinese maintainer if this translation is outdated
> +or if there is a problem with the translation.
> +
> +Maintainer: Punit Agrawal
> +Suzuki K. Poulose
> +Chinese maintainer: Fu Wei
> +-
> +Documentation/arm64/legacy_instructions.txt 的中文翻译
> +
> +如果想评论或更新本文的内容，请直接联系原文档的维护者。如果你使用英文
> +交流有困难的话，也可以向中文版维护者求助。如果本翻译更新不及时或者翻
> +译存在问题，请联系中文版维护者。
> +
> +本文翻译提交时的 Git 检出点为： bc465aa9d045feb0e13b4a8f32cc33c1943f62d6
> +
> +英文版维护者： Punit Agrawal
> +Suzuki K. Poulose
> +中文版维护者： 傅炜 Fu Wei
> +中文版翻译者： 傅炜 Fu Wei
> +中文版校译者： 傅炜 Fu Wei
> +
> +以下为正文
> +-
> +Linux 内核在 arm64 上的移植提供了一个基础框架，以支持构架中正在被淘汰或已废弃指令的模拟执行。
> +这个基础框架的代码使用未定义指令钩子（hooks）来支持模拟。如果指令存在，它也允许在硬件中启用该指令。
> +
> +模拟模式可通过写 sysctl 节点（/proc/sys/abi）来控制。
> +不同的执行方式及 sysctl 节点的相应值，解释如下：
> +
> +* Undef（未定义）
> + 值： 0
> + 产生未定义指令终止异常。它是那些构架中已废弃的指令，如 SWP，的默认处理方式。
> +
> +* Emulate（模拟）
> + 值： 1
> + 使用软件模拟方式。为解决软件迁移问题，这种模拟指令模式的使用是被跟踪的，并会发出速率限制警告。
> + 它是那些构架中正在被淘汰的指令，如 CP15 barriers（隔离指令），的默认处理方式。
> +
> +* Hardware Execution（硬件执行）
> + 值： 2
> + 虽然标记为正在被淘汰，但一些实现可能提供硬件执行这些指令的使能/禁用操作。
> + 使用硬件执行一般会有更好的性能，但将无法收集运行时对正被淘汰指令的使用统计数据。
> +
> +默认执行模式依赖于指令在构架中状态。正在被淘汰的指令应该以模拟（Emulate）作为默认模式，
> +而已废弃的指令必须默认使用未定义（Undef）模式
> +
> +注意：指令模拟可能无法应对所有情况。更多详情请参考单独的指令注释。
> +
> +受支持的遗留指令
> +-
> +* SWP{B}
> +节点: /proc/sys/abi/swp
> +状态: 已废弃
> +默认执行方式: Undef (0)
> +
> +* CP15 Barriers
> +节点: /proc/sys/abi/cp15_barrier
> +状态: 正被淘汰，不推荐使用
> +默认执行方式: Emulate (1)
> +
> +* SETEND
> +节点: /proc/sys/abi/setend
> +状态: 正被淘汰，不推荐使用
> +默认执行方式: Emulate (1)*
> +注：为了使能这个特性，系统中的所有 CPU 必须在 EL0 支持混合字节序。
> +如果一个新的 CPU （不支持混合字节序） 在使能这个特性后被热插入系统，
> +在应用中可能会出现不可预期的结果。
> --
> 1.8.3.1
--
Amos.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Documentation:Chinese translation of Documentation/arm64/legacy_instructions.txt
On Wed, Mar 25, 2015 at 12:09:03PM +0800, w...@redhat.com wrote:
> From: Fu Wei
>
> This is a Chinese translated version of
> Documentation/arm64/legacy_instructions.txt
>
> It is based on the modifications of
> Documentation/arm64/legacy_instructions.txt in submission:
> "587064b6", "bd35a4ad", "2d888f48", "c852f320".
Hi Fu wei,
> Signed-off-by: Fu Wei
> ---
> Documentation/zh_CN/arm64/legacy_instructions.txt | 72
> +++
> 1 file changed, 72 insertions(+)
>
> diff --git a/Documentation/zh_CN/arm64/legacy_instructions.txt
> b/Documentation/zh_CN/arm64/legacy_instructions.txt
> new file mode 100644
> index 000..75b11bf
> --- /dev/null
> +++ b/Documentation/zh_CN/arm64/legacy_instructions.txt
> @@ -0,0 +1,72 @@
> +Chinese translated version of Documentation/arm64/legacy_instructions.txt
> +
> +If you have any comment or update to the content, please contact the
> +original document maintainer directly. However, if you have a problem
> +communicating in English you can also ask the Chinese maintainer for
> +help. Contact the Chinese maintainer if this translation is outdated
> +or if there is a problem with the translation.
> +
> +Maintainer: Punit Agrawal
> +Suzuki K. Poulose
> +Chinese maintainer: Fu Wei
> +-
> +Documentation/arm64/legacy_instructions.txt 的中文翻译
> +
> +如果想评论或更新本文的内容，请直接联系原文档的维护者。如果你使用英文
> +交流有困难的话，也可以向中文版维护者求助。如果本翻译更新不及时或者翻
> +译存在问题，请联系中文版维护者。
> +
> +本文翻译提交时的 Git 检出点为： bc465aa9d045feb0e13b4a8f32cc33c1943f62d6
> +
> +英文版维护者： Punit Agrawal
> +Suzuki K. Poulose
> +中文版维护者： 傅炜 Fu Wei
> +中文版翻译者： 傅炜 Fu Wei
> +中文版校译者： 傅炜 Fu Wei
> +
> +以下为正文
> +-
> +Linux 内核在 arm64 上的移植提供了一个基础框架，以支持构架中正在被淘汰或已废弃指令的模拟执行。
> +这个基础框架的代码使用未定义指令钩子（hooks）来支持模拟。如果指令存在，它也允许在硬件中启用该指令。
> +
> +模拟模式可通过写 sysctl 节点（/proc/sys/abi）来控制。
> +不同的执行方式及 sysctl 节点的相应值，解释如下：
> +
> +* Undef（未定义）
> + 值： 0
> + 产生未定义指令终止异常。它是那些构架中已废弃的指令，如 SWP，的默认处理方式。
> +
> +* Emulate（模拟）
> + 值： 1
> + 使用软件模拟方式。为解决软件迁移问题，这种模拟指令模式的使用是被跟踪的，并会发出速率限制警告。
> + 它是那些构架中正在被淘汰的指令，如 CP15 barriers（隔离指令），的默认处理方式。
> +
> +* Hardware Execution（硬件执行）
> + 值： 2
> + 虽然标记为正在被淘汰，但一些实现可能提供硬件执行这些指令的使能/禁用操作。
> + 使用硬件执行一般会有更好的性能，但将无法收集运行时对正被淘汰指令的使用统计数据。
> +
> +默认执行模式依赖于指令在构架中状态。正在被淘汰的指令应该以模拟（Emulate）作为默认模式，
> +而已废弃的指令必须默认使用未定义（Undef）模式
> +
> +注意：指令模拟可能无法应对所有情况。更多详情请参考单独的指令注释。
> +
> +受支持的遗留指令
> +-
> +* SWP{B}
> +节点: /proc/sys/abi/swp
> +状态: 已废弃
> +默认执行方式: Undef (0)
> +
> +* CP15 Barriers
> +节点: /proc/sys/abi/cp15_barrier
> +状态: 正被淘汰，不推荐使用
> +默认执行方式: Emulate (1)
> +
> +* SETEND
> +节点: /proc/sys/abi/setend
> +状态: 正被淘汰，不推荐使用
> +默认执行方式: Emulate (1)*
> +注：为了使能这个特性，所有系统中的 CPU 必须在 EL0 支持混合字节序。
raw content: All the cpus on the system
Your translation might mean "CPUs on all the system", translating to
'系统中的所有CPU' is better.
> +如果一个新的 CPU （不支持混合字节序） 在使能这个特性后被热插入系统，
> +该应用则可能出现不可预期的结果。
'该应用' 'the application' is not a specific application.
how about '在应用中可能会出现' ?
--
Amos.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v5 REPOST 1/6] hw_random: place mutex around read functions and buffers.
From: Rusty Russell
There's currently a big lock around everything, and it means that we
can't query sysfs (eg /sys/devices/virtual/misc/hw_random/rng_current)
while the rng is reading. This is a real problem when the rng is slow,
or blocked (eg. virtio_rng with qemu's default /dev/random backend)
This doesn't help (it leaves the current lock untouched), just adds a
lock to protect the read function and the static buffers, in preparation
for transition.
Signed-off-by: Rusty Russell
---
drivers/char/hw_random/core.c | 20 +---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index aa30a25..b1b6042 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -53,7 +53,10 @@
static struct hwrng *current_rng;
static struct task_struct *hwrng_fill;
static LIST_HEAD(rng_list);
+/* Protects rng_list and current_rng */
static DEFINE_MUTEX(rng_mutex);
+/* Protects rng read functions, data_avail, rng_buffer and rng_fillbuf */
+static DEFINE_MUTEX(reading_mutex);
static int data_avail;
static u8 *rng_buffer, *rng_fillbuf;
static unsigned short current_quality;
@@ -81,7 +84,9 @@ static void add_early_randomness(struct hwrng *rng)
unsigned char bytes[16];
int bytes_read;
+ mutex_lock(&reading_mutex);
bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
+ mutex_unlock(&reading_mutex);
if (bytes_read > 0)
add_device_randomness(bytes, bytes_read);
}
@@ -128,6 +133,7 @@ static inline int rng_get_data(struct hwrng *rng, u8
*buffer, size_t size,
int wait) {
int present;
+ BUG_ON(!mutex_is_locked(&reading_mutex));
if (rng->read)
return rng->read(rng, (void *)buffer, size, wait);
@@ -160,13 +166,14 @@ static ssize_t rng_dev_read(struct file *filp, char
__user *buf,
goto out_unlock;
}
+ mutex_lock(&reading_mutex);
if (!data_avail) {
bytes_read = rng_get_data(current_rng, rng_buffer,
rng_buffer_size(),
!(filp->f_flags & O_NONBLOCK));
if (bytes_read < 0) {
err = bytes_read;
- goto out_unlock;
+ goto out_unlock_reading;
}
data_avail = bytes_read;
}
@@ -174,7 +181,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user
*buf,
if (!data_avail) {
if (filp->f_flags & O_NONBLOCK) {
err = -EAGAIN;
- goto out_unlock;
+ goto out_unlock_reading;
}
} else {
len = data_avail;
@@ -186,7 +193,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user
*buf,
if (copy_to_user(buf + ret, rng_buffer + data_avail,
len)) {
err = -EFAULT;
- goto out_unlock;
+ goto out_unlock_reading;
}
size -= len;
@@ -194,6 +201,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user
*buf,
}
mutex_unlock(&rng_mutex);
+ mutex_unlock(&reading_mutex);
if (need_resched())
schedule_timeout_interruptible(1);
@@ -208,6 +216,9 @@ out:
out_unlock:
mutex_unlock(&rng_mutex);
goto out;
+out_unlock_reading:
+ mutex_unlock(&reading_mutex);
+ goto out_unlock;
}
@@ -348,13 +359,16 @@ static int hwrng_fillfn(void *unused)
while (!kthread_should_stop()) {
if (!current_rng)
break;
+ mutex_lock(&reading_mutex);
rc = rng_get_data(current_rng, rng_fillbuf,
rng_buffer_size(), 1);
+ mutex_unlock(&reading_mutex);
if (rc <= 0) {
pr_warn("hwrng: no data available\n");
msleep_interruptible(1);
continue;
}
+ /* Outside lock, sure, but y'know: randomness. */
add_hwgenerator_randomness((void *)rng_fillbuf, rc,
rc * current_quality * 8 >> 10);
}
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v5 REPOST 4/6] hw_random: fix unregister race.
From: Rusty Russell
The previous patch added one potential problem: we can still be
reading from a hwrng when it's unregistered. Add a wait for zero
in the hwrng_unregister path.
v5: reset cleanup_done flag, use compiler barrier to prevent recording.
v4: add cleanup_done flag to insure that cleanup is done
Signed-off-by: Rusty Russell
Signed-off-by: Amos Kong
---
drivers/char/hw_random/core.c | 12
include/linux/hw_random.h | 1 +
2 files changed, 13 insertions(+)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 83516cb..067270b 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -60,6 +60,7 @@ static DEFINE_MUTEX(rng_mutex);
static DEFINE_MUTEX(reading_mutex);
static int data_avail;
static u8 *rng_buffer, *rng_fillbuf;
+static DECLARE_WAIT_QUEUE_HEAD(rng_done);
static unsigned short current_quality;
static unsigned short default_quality; /* = 0; default to "off" */
@@ -98,6 +99,11 @@ static inline void cleanup_rng(struct kref *kref)
if (rng->cleanup)
rng->cleanup(rng);
+
+ /* cleanup_done should be updated after cleanup finishes */
+ smp_wmb();
+ rng->cleanup_done = true;
+ wake_up_all(&rng_done);
}
static void set_current_rng(struct hwrng *rng)
@@ -498,6 +504,8 @@ int hwrng_register(struct hwrng *rng)
add_early_randomness(rng);
}
+ rng->cleanup_done = false;
+
out_unlock:
mutex_unlock(&rng_mutex);
out:
@@ -529,6 +537,10 @@ void hwrng_unregister(struct hwrng *rng)
kthread_stop(hwrng_fill);
} else
mutex_unlock(&rng_mutex);
+
+ /* Just in case rng is reading right now, wait. */
+ wait_event(rng_done, rng->cleanup_done &&
+ atomic_read(&rng->ref.refcount) == 0);
}
EXPORT_SYMBOL_GPL(hwrng_unregister);
diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h
index c212e71..7832e50 100644
--- a/include/linux/hw_random.h
+++ b/include/linux/hw_random.h
@@ -46,6 +46,7 @@ struct hwrng {
/* internal. */
struct list_head list;
struct kref ref;
+ bool cleanup_done;
};
/** Register a new Hardware Random Number Generator driver. */
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v5 REPOST 5/6] hw_random: don't double-check old_rng.
From: Rusty Russell
Interesting anti-pattern.
Signed-off-by: Rusty Russell
---
drivers/char/hw_random/core.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 067270b..a9286bf 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -476,14 +476,13 @@ int hwrng_register(struct hwrng *rng)
}
old_rng = current_rng;
+ err = 0;
if (!old_rng) {
err = hwrng_init(rng);
if (err)
goto out_unlock;
set_current_rng(rng);
- }
- err = 0;
- if (!old_rng) {
+
err = register_miscdev();
if (err) {
drop_current_rng();
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v5 REPOST 0/6] fix hw_random stuck
When I hotunplug a busy virtio-rng device or try to access hwrng attributes in non-smp guest, it gets stuck. My hotplug tests: | test 0: | hotunplug rng device from qemu monitor | | test 1: | guest) # dd if=/dev/hwrng of=/dev/null & | hotunplug rng device from qemu monitor | | test 2: | guest) # dd if=/dev/random of=/dev/null & | hotunplug rng device from qemu monitor | | test 4: | guest) # dd if=/dev/hwrng of=/dev/null & | cat /sys/devices/virtual/misc/hw_random/rng_* | | test 5: | 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 | | test 6: | use a fifo as rng backend, execute test 0 ~ 5 with no input of fifo V5: reset cleanup_done flag, drop redundant init of reference count, use compiler barrier to prevent recording. V4: update patch 4 to fix corrupt, decrease last reference for triggering the cleanup, fix unregister race pointed by Herbert V3: initialize kref to 1 V2: added patch 2 to fix a deadlock, update current patch 3 to fix reference counting issue Amos Kong (1): hw_random: move some code out mutex_lock for avoiding underlying deadlock Rusty Russell (5): hw_random: place mutex around read functions and buffers. hw_random: use reference counts on each struct hwrng. hw_random: fix unregister race. hw_random: don't double-check old_rng. hw_random: don't init list element we're about to add to list. drivers/char/hw_random/core.c | 173 ++ include/linux/hw_random.h | 3 + 2 files changed, 126 insertions(+), 50 deletions(-) -- 1.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v5 REPOST 3/6] hw_random: use reference counts on each struct hwrng.
From: Rusty Russell
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.
v5: drop redundant kref_init()
v4: decrease last reference for triggering the cleanup
v3: initialize kref (thanks Amos Kong)
v2: fix missing put_rng() on exit path (thanks Amos Kong)
Signed-off-by: Rusty Russell
Signed-off-by: Amos Kong
---
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 a0905c8..83516cb 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,60 @@ 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;
+
+ /* decrease last reference for triggering the cleanup */
+ kref_put(¤t_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 +168,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 +203,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,8 +250,8 @@ static ssize_t rng_dev_read(struct file *filp, char __user
*buf,
ret += len;
}
- mutex_unlock(&rng_mutex);
mutex_unlock(&reading_mutex);
+ put_rng(rng);
if (need_resched())
schedule_timeout_interruptible(1);
@@ -213,12 +263,11 @@ static ssize_t rng_dev_read(struct file *filp, char
__user *buf,
}
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_at
[PATCH v5 REPOST 6/6] hw_random: don't init list element we're about to add to list.
From: Rusty Russell
Another interesting anti-pattern.
Signed-off-by: Rusty Russell
---
drivers/char/hw_random/core.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index a9286bf..4d13ac5 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -489,7 +489,6 @@ int hwrng_register(struct hwrng *rng)
goto out_unlock;
}
}
- INIT_LIST_HEAD(&rng->list);
list_add_tail(&rng->list, &rng_list);
if (old_rng && !rng->init) {
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v5 REPOST 2/6] hw_random: move some code out mutex_lock for avoiding underlying deadlock
In next patch, we use reference counting for each struct hwrng,
changing reference count also needs to take mutex_lock. Before
releasing the lock, if we try to stop a kthread that waits to
take the lock to reduce the referencing count, deadlock will
occur.
Signed-off-by: Amos Kong
---
drivers/char/hw_random/core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index b1b6042..a0905c8 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -474,12 +474,12 @@ void hwrng_unregister(struct hwrng *rng)
}
}
if (list_empty(&rng_list)) {
+ mutex_unlock(&rng_mutex);
unregister_miscdev();
if (hwrng_fill)
kthread_stop(hwrng_fill);
- }
-
- mutex_unlock(&rng_mutex);
+ } else
+ mutex_unlock(&rng_mutex);
}
EXPORT_SYMBOL_GPL(hwrng_unregister);
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v5 0/6] fix hw_random stuck
When I hotunplug a busy virtio-rng device or try to access hwrng attributes in non-smp guest, it gets stuck. My hotplug tests: | test 0: | hotunplug rng device from qemu monitor | | test 1: | guest) # dd if=/dev/hwrng of=/dev/null & | hotunplug rng device from qemu monitor | | test 2: | guest) # dd if=/dev/random of=/dev/null & | hotunplug rng device from qemu monitor | | test 4: | guest) # dd if=/dev/hwrng of=/dev/null & | cat /sys/devices/virtual/misc/hw_random/rng_* | | test 5: | 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 | | test 6: | use a fifo as rng backend, execute test 0 ~ 5 with no input of fifo V5: reset cleanup_done flag, drop redundant init of reference count, use compiler barrier to prevent recording. V4: update patch 4 to fix corrupt, decrease last reference for triggering the cleanup, fix unregister race pointed by Herbert V3: initialize kref to 1 V2: added patch 2 to fix a deadlock, update current patch 3 to fix reference counting issue Amos Kong (1): hw_random: move some code out mutex_lock for avoiding underlying deadlock Rusty Russell (5): hw_random: place mutex around read functions and buffers. hw_random: use reference counts on each struct hwrng. hw_random: fix unregister race. hw_random: don't double-check old_rng. hw_random: don't init list element we're about to add to list. drivers/char/hw_random/core.c | 173 ++ include/linux/hw_random.h | 3 + 2 files changed, 126 insertions(+), 50 deletions(-) -- 1.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v5 3/6] hw_random: use reference counts on each struct hwrng.
From: Rusty Russell
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.
v5: drop redundant kref_init()
v4: decrease last reference for triggering the cleanup
v3: initialize kref (thanks Amos Kong)
v2: fix missing put_rng() on exit path (thanks Amos Kong)
Signed-off-by: Rusty Russell
Signed-off-by: Amos Kong
---
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 a0905c8..83516cb 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,60 @@ 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;
+
+ /* decrease last reference for triggering the cleanup */
+ kref_put(¤t_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 +168,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 +203,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,8 +250,8 @@ static ssize_t rng_dev_read(struct file *filp, char __user
*buf,
ret += len;
}
- mutex_unlock(&rng_mutex);
mutex_unlock(&reading_mutex);
+ put_rng(rng);
if (need_resched())
schedule_timeout_interruptible(1);
@@ -213,12 +263,11 @@ static ssize_t rng_dev_read(struct file *filp, char
__user *buf,
}
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_at
[PATCH v5 6/6] hw_random: don't init list element we're about to add to list.
From: Rusty Russell
Another interesting anti-pattern.
Signed-off-by: Rusty Russell
---
drivers/char/hw_random/core.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index a9286bf..4d13ac5 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -489,7 +489,6 @@ int hwrng_register(struct hwrng *rng)
goto out_unlock;
}
}
- INIT_LIST_HEAD(&rng->list);
list_add_tail(&rng->list, &rng_list);
if (old_rng && !rng->init) {
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v5 5/6] hw_random: don't double-check old_rng.
From: Rusty Russell
Interesting anti-pattern.
Signed-off-by: Rusty Russell
---
drivers/char/hw_random/core.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 067270b..a9286bf 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -476,14 +476,13 @@ int hwrng_register(struct hwrng *rng)
}
old_rng = current_rng;
+ err = 0;
if (!old_rng) {
err = hwrng_init(rng);
if (err)
goto out_unlock;
set_current_rng(rng);
- }
- err = 0;
- if (!old_rng) {
+
err = register_miscdev();
if (err) {
drop_current_rng();
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v5 2/6] hw_random: move some code out mutex_lock for avoiding underlying deadlock
In next patch, we use reference counting for each struct hwrng,
changing reference count also needs to take mutex_lock. Before
releasing the lock, if we try to stop a kthread that waits to
take the lock to reduce the referencing count, deadlock will
occur.
Signed-off-by: Amos Kong
---
drivers/char/hw_random/core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index b1b6042..a0905c8 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -474,12 +474,12 @@ void hwrng_unregister(struct hwrng *rng)
}
}
if (list_empty(&rng_list)) {
+ mutex_unlock(&rng_mutex);
unregister_miscdev();
if (hwrng_fill)
kthread_stop(hwrng_fill);
- }
-
- mutex_unlock(&rng_mutex);
+ } else
+ mutex_unlock(&rng_mutex);
}
EXPORT_SYMBOL_GPL(hwrng_unregister);
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v5 1/6] hw_random: place mutex around read functions and buffers.
From: Rusty Russell
There's currently a big lock around everything, and it means that we
can't query sysfs (eg /sys/devices/virtual/misc/hw_random/rng_current)
while the rng is reading. This is a real problem when the rng is slow,
or blocked (eg. virtio_rng with qemu's default /dev/random backend)
This doesn't help (it leaves the current lock untouched), just adds a
lock to protect the read function and the static buffers, in preparation
for transition.
Signed-off-by: Rusty Russell
---
drivers/char/hw_random/core.c | 20 +---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index aa30a25..b1b6042 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -53,7 +53,10 @@
static struct hwrng *current_rng;
static struct task_struct *hwrng_fill;
static LIST_HEAD(rng_list);
+/* Protects rng_list and current_rng */
static DEFINE_MUTEX(rng_mutex);
+/* Protects rng read functions, data_avail, rng_buffer and rng_fillbuf */
+static DEFINE_MUTEX(reading_mutex);
static int data_avail;
static u8 *rng_buffer, *rng_fillbuf;
static unsigned short current_quality;
@@ -81,7 +84,9 @@ static void add_early_randomness(struct hwrng *rng)
unsigned char bytes[16];
int bytes_read;
+ mutex_lock(&reading_mutex);
bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
+ mutex_unlock(&reading_mutex);
if (bytes_read > 0)
add_device_randomness(bytes, bytes_read);
}
@@ -128,6 +133,7 @@ static inline int rng_get_data(struct hwrng *rng, u8
*buffer, size_t size,
int wait) {
int present;
+ BUG_ON(!mutex_is_locked(&reading_mutex));
if (rng->read)
return rng->read(rng, (void *)buffer, size, wait);
@@ -160,13 +166,14 @@ static ssize_t rng_dev_read(struct file *filp, char
__user *buf,
goto out_unlock;
}
+ mutex_lock(&reading_mutex);
if (!data_avail) {
bytes_read = rng_get_data(current_rng, rng_buffer,
rng_buffer_size(),
!(filp->f_flags & O_NONBLOCK));
if (bytes_read < 0) {
err = bytes_read;
- goto out_unlock;
+ goto out_unlock_reading;
}
data_avail = bytes_read;
}
@@ -174,7 +181,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user
*buf,
if (!data_avail) {
if (filp->f_flags & O_NONBLOCK) {
err = -EAGAIN;
- goto out_unlock;
+ goto out_unlock_reading;
}
} else {
len = data_avail;
@@ -186,7 +193,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user
*buf,
if (copy_to_user(buf + ret, rng_buffer + data_avail,
len)) {
err = -EFAULT;
- goto out_unlock;
+ goto out_unlock_reading;
}
size -= len;
@@ -194,6 +201,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user
*buf,
}
mutex_unlock(&rng_mutex);
+ mutex_unlock(&reading_mutex);
if (need_resched())
schedule_timeout_interruptible(1);
@@ -208,6 +216,9 @@ out:
out_unlock:
mutex_unlock(&rng_mutex);
goto out;
+out_unlock_reading:
+ mutex_unlock(&reading_mutex);
+ goto out_unlock;
}
@@ -348,13 +359,16 @@ static int hwrng_fillfn(void *unused)
while (!kthread_should_stop()) {
if (!current_rng)
break;
+ mutex_lock(&reading_mutex);
rc = rng_get_data(current_rng, rng_fillbuf,
rng_buffer_size(), 1);
+ mutex_unlock(&reading_mutex);
if (rc <= 0) {
pr_warn("hwrng: no data available\n");
msleep_interruptible(1);
continue;
}
+ /* Outside lock, sure, but y'know: randomness. */
add_hwgenerator_randomness((void *)rng_fillbuf, rc,
rc * current_quality * 8 >> 10);
}
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v5 4/6] hw_random: fix unregister race.
From: Rusty Russell
The previous patch added one potential problem: we can still be
reading from a hwrng when it's unregistered. Add a wait for zero
in the hwrng_unregister path.
v5: reset cleanup_done flag, use compiler barrier to prevent recording.
v4: add cleanup_done flag to insure that cleanup is done
Signed-off-by: Rusty Russell
Signed-off-by: Amos Kong
---
drivers/char/hw_random/core.c | 12
include/linux/hw_random.h | 1 +
2 files changed, 13 insertions(+)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 83516cb..067270b 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -60,6 +60,7 @@ static DEFINE_MUTEX(rng_mutex);
static DEFINE_MUTEX(reading_mutex);
static int data_avail;
static u8 *rng_buffer, *rng_fillbuf;
+static DECLARE_WAIT_QUEUE_HEAD(rng_done);
static unsigned short current_quality;
static unsigned short default_quality; /* = 0; default to "off" */
@@ -98,6 +99,11 @@ static inline void cleanup_rng(struct kref *kref)
if (rng->cleanup)
rng->cleanup(rng);
+
+ /* cleanup_done should be updated after cleanup finishes */
+ smp_wmb();
+ rng->cleanup_done = true;
+ wake_up_all(&rng_done);
}
static void set_current_rng(struct hwrng *rng)
@@ -498,6 +504,8 @@ int hwrng_register(struct hwrng *rng)
add_early_randomness(rng);
}
+ rng->cleanup_done = false;
+
out_unlock:
mutex_unlock(&rng_mutex);
out:
@@ -529,6 +537,10 @@ void hwrng_unregister(struct hwrng *rng)
kthread_stop(hwrng_fill);
} else
mutex_unlock(&rng_mutex);
+
+ /* Just in case rng is reading right now, wait. */
+ wait_event(rng_done, rng->cleanup_done &&
+ atomic_read(&rng->ref.refcount) == 0);
}
EXPORT_SYMBOL_GPL(hwrng_unregister);
diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h
index c212e71..7832e50 100644
--- a/include/linux/hw_random.h
+++ b/include/linux/hw_random.h
@@ -46,6 +46,7 @@ struct hwrng {
/* internal. */
struct list_head list;
struct kref ref;
+ bool cleanup_done;
};
/** Register a new Hardware Random Number Generator driver. */
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 4/6] hw_random: fix unregister race.
On Wed, Nov 12, 2014 at 02:33:00PM +1030, Rusty Russell wrote: > Amos Kong writes: > > From: Rusty Russell > > > > The previous patch added one potential problem: we can still be > > reading from a hwrng when it's unregistered. Add a wait for zero > > in the hwrng_unregister path. > > > > v4: add cleanup_done flag to insure that cleanup is done > > That's a bit weird. The usual pattern would be to hold a reference > until we're actually finished, but this reference is a bit weird. > > We hold the mutex across cleanup, so we could grab that but we have to > take care sleeping inside wait_event, otherwise Peter will have to fix > my code again :) > > AFAICT the wake_woken() stuff isn't merged yet, so your patch will > have to do for now. > > > @@ -98,6 +99,8 @@ static inline void cleanup_rng(struct kref *kref) > > > > if (rng->cleanup) > > rng->cleanup(rng); > > + rng->cleanup_done = true; > > + wake_up_all(&rng_done); > > } > > > > static void set_current_rng(struct hwrng *rng) > > @@ -536,6 +539,11 @@ void hwrng_unregister(struct hwrng *rng) > > kthread_stop(hwrng_fill); > > } else > > mutex_unlock(&rng_mutex); > > + > > + /* Just in case rng is reading right now, wait. */ > > + wait_event(rng_done, rng->cleanup_done && > > + atomic_read(&rng->ref.refcount) == 0); > > + > > The atomic_read() isn't necessary here. At least, we need it to convert refcount from atomic_t to int. Otherwise, we will touch this error: error: invalid operands to binary == (have 'atomic_t' and 'int') > However, you should probably init cleanup_done in hwrng_register(). > (Probably noone does unregister then register, but let's be clear). > > Thanks, > Rusty. -- Amos. signature.asc Description: Digital signature
Re: [PATCH v4 4/6] hw_random: fix unregister race.
On Wed, Nov 12, 2014 at 02:33:00PM +1030, Rusty Russell wrote:
> Amos Kong writes:
> > From: Rusty Russell
> >
> > The previous patch added one potential problem: we can still be
> > reading from a hwrng when it's unregistered. Add a wait for zero
> > in the hwrng_unregister path.
> >
> > v4: add cleanup_done flag to insure that cleanup is done
>
> That's a bit weird. The usual pattern would be to hold a reference
> until we're actually finished, but this reference is a bit weird.
The cleanup function is a callback function of kref_put(), we can't
use the same reference count inside cleanup function.
> We hold the mutex across cleanup, so we could grab that but we have to
> take care sleeping inside wait_event, otherwise Peter will have to fix
> my code again :)
We didn't hold rng_mutex inside cleanup_rng(), am I missing something?
> AFAICT the wake_woken() stuff isn't merged yet, so your patch will
> have to do for now.
Can you provide some patches/mail link here? I searched nothing about
wake_woken.
> > @@ -98,6 +99,8 @@ static inline void cleanup_rng(struct kref *kref)
> >
> > if (rng->cleanup)
> > rng->cleanup(rng);
> > + rng->cleanup_done = true;
> > + wake_up_all(&rng_done);
> > }
> >
> > static void set_current_rng(struct hwrng *rng)
> > @@ -536,6 +539,11 @@ void hwrng_unregister(struct hwrng *rng)
> > kthread_stop(hwrng_fill);
> > } else
> > mutex_unlock(&rng_mutex);
> > +
> > + /* Just in case rng is reading right now, wait. */
> > + wait_event(rng_done, rng->cleanup_done &&
> > + atomic_read(&rng->ref.refcount) == 0);
> > +
>
> The atomic_read() isn't necessary here.
>
> However, you should probably init cleanup_done in hwrng_register().
> (Probably noone does unregister then register, but let's be clear).
Got it.
> Thanks,
> Rusty.
>
> > }
> > EXPORT_SYMBOL_GPL(hwrng_unregister);
> >
> > diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h
> > index c212e71..7832e50 100644
> > --- a/include/linux/hw_random.h
> > +++ b/include/linux/hw_random.h
> > @@ -46,6 +46,7 @@ struct hwrng {
> > /* internal. */
> > struct list_head list;
> > struct kref ref;
> > + bool cleanup_done;
> > };
> >
> > /** Register a new Hardware Random Number Generator driver. */
> > --
> > 1.9.3
--
Amos.
signature.asc
Description: Digital signature
Re: [PATCH net-next] tun: return NET_XMIT_DROP for dropped packets
On Tue, Nov 18, 2014 at 01:20:41PM +0800, Jason Wang wrote:
> After commit 5d097109257c03a71845729f8db6b5770c4bbedc
> ("tun: only queue packets on device"), NETDEV_TX_OK was returned for
> dropped packets. This will confuse pktgen since dropped packets were
> counted as sent ones.
>
> Fixing this by returning NET_XMIT_DROP to let pktgen count it as error
> packet.
>
> Cc: Michael S. Tsirkin
> Signed-off-by: Jason Wang
> ---
> drivers/net/tun.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index e3fa65a..ac53a73 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -819,7 +819,7 @@ drop:
> skb_tx_error(skb);
> kfree_skb(skb);
> rcu_read_unlock();
> - return NETDEV_TX_OK;
> + return NET_XMIT_DROP;
Quoted from linux/drivers/firewire/net.c:
/*
* FIXME: According to a patch from 2003-02-26, "returning non-zero
* causes serious problems" here, allegedly. Before that patch,
* -ERRNO was returned which is not appropriate under Linux 2.6.
* Perhaps more needs to be done? Stop the queue in serious
* conditions and restart it elsewhere?
*/
I saw many drivers return NETDEV_TX_OK in xmit for drop packets, eg:
virtio_net.c
> }
>
> static void tun_net_mclist(struct net_device *dev)
> --
> 1.9.1
--
Amos.
signature.asc
Description: Digital signature
Re: [PATCH v4 3/6] hw_random: use reference counts on each struct hwrng.
On Wed, Nov 12, 2014 at 02:11:23PM +1030, Rusty Russell wrote:
> Amos Kong writes:
> > From: Rusty Russell
> >
> > 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.
> >
> > v4: decrease last reference for triggering the cleanup
>
> This doesn't make any sense:
>
> > +static void drop_current_rng(void)
> > +{
> > + struct hwrng *rng = current_rng;
> > +
> > + BUG_ON(!mutex_is_locked(&rng_mutex));
> > + if (!current_rng)
> > + return;
> > +
> > + /* release current_rng reference */
> > + kref_put(¤t_rng->ref, cleanup_rng);
> > + current_rng = NULL;
> > +
> > + /* decrease last reference for triggering the cleanup */
> > + kref_put(&rng->ref, cleanup_rng);
> > +}
>
> Why would it drop the refcount twice? This doesn't make sense.
>
> Hmm, because you added kref_init, which initializes the reference count
> to 1, you created this bug.
I saw some kernel code uses kref_* helper functions, the reference
conter is initialized to 1. Some code didn't use the helper functions
to increase/decrease the reference counter. So I will drop kref_init()
and the second kref_put().
> Leave out the kref_init, and let it naturally be 0 (until, and if, it
> becomes current_rng). Add a comment if you want.
OK, thanks.
> Thanks,
> Rusty.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Amos.
signature.asc
Description: Digital signature
Re: [3.16 stable PATCH 0/2] virtio-rng: two backports to fix stuck
On Wed, Nov 05, 2014 at 04:02:49PM +, Luis Henriques wrote: > Hi Amos > > On Tue, Nov 04, 2014 at 12:32:27PM +0800, Amos Kong wrote: > > On Sat, Oct 11, 2014 at 06:51:47AM +0800, Amos Kong wrote: > > > I received two mails about faile to apply patches to 3.16-stable tree: > > > > > > FAILED: patch "[PATCH] virtio-rng: skip reading when we start to remove > > > the device" failed to apply to 3.16-stable tree > > > FAILED: patch "[PATCH] virtio-rng: fix stuck of hot-unplugging busy > > > device" failed to apply to 3.16-stable tree > > > > > > Amit already backported two patches for 3.16-stable, then cherry-pick > > > of my two patches works. > > > > > > Thanks. > > > > Ping Greg, thanks. > > > > I'm now doing the extended stable maintenance of the 3.16 kernel, as > Greg has EOL'ed it. > > Anyway, I will be queuing these 2 patches for the extended 3.16 > kernel. Thank you! Thanks a lot! > Cheers, > -- > Luís > > > > Amos Kong (2): > > > virtio-rng: fix stuck of hot-unplugging busy device > > > virtio-rng: skip reading when we start to remove the device > > > > > > drivers/char/hw_random/virtio-rng.c | 7 +++ > > > 1 file changed, 7 insertions(+) -- Amos. signature.asc Description: Digital signature
Re: [3.16 stable PATCH 0/2] virtio-rng: two backports to fix stuck
On Sat, Oct 11, 2014 at 06:51:47AM +0800, Amos Kong wrote: > I received two mails about faile to apply patches to 3.16-stable tree: > > FAILED: patch "[PATCH] virtio-rng: skip reading when we start to remove the > device" failed to apply to 3.16-stable tree > FAILED: patch "[PATCH] virtio-rng: fix stuck of hot-unplugging busy device" > failed to apply to 3.16-stable tree > > Amit already backported two patches for 3.16-stable, then cherry-pick > of my two patches works. > > Thanks. Ping Greg, thanks. > Amos Kong (2): > virtio-rng: fix stuck of hot-unplugging busy device > virtio-rng: skip reading when we start to remove the device > > drivers/char/hw_random/virtio-rng.c | 7 +++ > 1 file changed, 7 insertions(+) > > -- > 1.9.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- Amos. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 1/6] hw_random: place mutex around read functions and buffers.
From: Rusty Russell
There's currently a big lock around everything, and it means that we
can't query sysfs (eg /sys/devices/virtual/misc/hw_random/rng_current)
while the rng is reading. This is a real problem when the rng is slow,
or blocked (eg. virtio_rng with qemu's default /dev/random backend)
This doesn't help (it leaves the current lock untouched), just adds a
lock to protect the read function and the static buffers, in preparation
for transition.
Signed-off-by: Rusty Russell
---
drivers/char/hw_random/core.c | 20 +---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index aa30a25..b1b6042 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -53,7 +53,10 @@
static struct hwrng *current_rng;
static struct task_struct *hwrng_fill;
static LIST_HEAD(rng_list);
+/* Protects rng_list and current_rng */
static DEFINE_MUTEX(rng_mutex);
+/* Protects rng read functions, data_avail, rng_buffer and rng_fillbuf */
+static DEFINE_MUTEX(reading_mutex);
static int data_avail;
static u8 *rng_buffer, *rng_fillbuf;
static unsigned short current_quality;
@@ -81,7 +84,9 @@ static void add_early_randomness(struct hwrng *rng)
unsigned char bytes[16];
int bytes_read;
+ mutex_lock(&reading_mutex);
bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
+ mutex_unlock(&reading_mutex);
if (bytes_read > 0)
add_device_randomness(bytes, bytes_read);
}
@@ -128,6 +133,7 @@ static inline int rng_get_data(struct hwrng *rng, u8
*buffer, size_t size,
int wait) {
int present;
+ BUG_ON(!mutex_is_locked(&reading_mutex));
if (rng->read)
return rng->read(rng, (void *)buffer, size, wait);
@@ -160,13 +166,14 @@ static ssize_t rng_dev_read(struct file *filp, char
__user *buf,
goto out_unlock;
}
+ mutex_lock(&reading_mutex);
if (!data_avail) {
bytes_read = rng_get_data(current_rng, rng_buffer,
rng_buffer_size(),
!(filp->f_flags & O_NONBLOCK));
if (bytes_read < 0) {
err = bytes_read;
- goto out_unlock;
+ goto out_unlock_reading;
}
data_avail = bytes_read;
}
@@ -174,7 +181,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user
*buf,
if (!data_avail) {
if (filp->f_flags & O_NONBLOCK) {
err = -EAGAIN;
- goto out_unlock;
+ goto out_unlock_reading;
}
} else {
len = data_avail;
@@ -186,7 +193,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user
*buf,
if (copy_to_user(buf + ret, rng_buffer + data_avail,
len)) {
err = -EFAULT;
- goto out_unlock;
+ goto out_unlock_reading;
}
size -= len;
@@ -194,6 +201,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user
*buf,
}
mutex_unlock(&rng_mutex);
+ mutex_unlock(&reading_mutex);
if (need_resched())
schedule_timeout_interruptible(1);
@@ -208,6 +216,9 @@ out:
out_unlock:
mutex_unlock(&rng_mutex);
goto out;
+out_unlock_reading:
+ mutex_unlock(&reading_mutex);
+ goto out_unlock;
}
@@ -348,13 +359,16 @@ static int hwrng_fillfn(void *unused)
while (!kthread_should_stop()) {
if (!current_rng)
break;
+ mutex_lock(&reading_mutex);
rc = rng_get_data(current_rng, rng_fillbuf,
rng_buffer_size(), 1);
+ mutex_unlock(&reading_mutex);
if (rc <= 0) {
pr_warn("hwrng: no data available\n");
msleep_interruptible(1);
continue;
}
+ /* Outside lock, sure, but y'know: randomness. */
add_hwgenerator_randomness((void *)rng_fillbuf, rc,
rc * current_quality * 8 >> 10);
}
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 5/6] hw_random: don't double-check old_rng.
From: Rusty Russell
Interesting anti-pattern.
Signed-off-by: Rusty Russell
---
drivers/char/hw_random/core.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index c31bf91..fc5de7d 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -480,14 +480,13 @@ int hwrng_register(struct hwrng *rng)
}
old_rng = current_rng;
+ err = 0;
if (!old_rng) {
err = hwrng_init(rng);
if (err)
goto out_unlock;
set_current_rng(rng);
- }
- err = 0;
- if (!old_rng) {
+
err = register_miscdev();
if (err) {
drop_current_rng();
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 4/6] hw_random: fix unregister race.
From: Rusty Russell
The previous patch added one potential problem: we can still be
reading from a hwrng when it's unregistered. Add a wait for zero
in the hwrng_unregister path.
v4: add cleanup_done flag to insure that cleanup is done
Signed-off-by: Rusty Russell
Signed-off-by: Amos Kong
---
drivers/char/hw_random/core.c | 8
include/linux/hw_random.h | 1 +
2 files changed, 9 insertions(+)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 27ad6b4..c31bf91 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -60,6 +60,7 @@ static DEFINE_MUTEX(rng_mutex);
static DEFINE_MUTEX(reading_mutex);
static int data_avail;
static u8 *rng_buffer, *rng_fillbuf;
+static DECLARE_WAIT_QUEUE_HEAD(rng_done);
static unsigned short current_quality;
static unsigned short default_quality; /* = 0; default to "off" */
@@ -98,6 +99,8 @@ static inline void cleanup_rng(struct kref *kref)
if (rng->cleanup)
rng->cleanup(rng);
+ rng->cleanup_done = true;
+ wake_up_all(&rng_done);
}
static void set_current_rng(struct hwrng *rng)
@@ -536,6 +539,11 @@ void hwrng_unregister(struct hwrng *rng)
kthread_stop(hwrng_fill);
} else
mutex_unlock(&rng_mutex);
+
+ /* Just in case rng is reading right now, wait. */
+ wait_event(rng_done, rng->cleanup_done &&
+ atomic_read(&rng->ref.refcount) == 0);
+
}
EXPORT_SYMBOL_GPL(hwrng_unregister);
diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h
index c212e71..7832e50 100644
--- a/include/linux/hw_random.h
+++ b/include/linux/hw_random.h
@@ -46,6 +46,7 @@ struct hwrng {
/* internal. */
struct list_head list;
struct kref ref;
+ bool cleanup_done;
};
/** Register a new Hardware Random Number Generator driver. */
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 2/6] hw_random: move some code out mutex_lock for avoiding underlying deadlock
In next patch, we use reference counting for each struct hwrng,
changing reference count also needs to take mutex_lock. Before
releasing the lock, if we try to stop a kthread that waits to
take the lock to reduce the referencing count, deadlock will
occur.
Signed-off-by: Amos Kong
---
drivers/char/hw_random/core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index b1b6042..a0905c8 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -474,12 +474,12 @@ void hwrng_unregister(struct hwrng *rng)
}
}
if (list_empty(&rng_list)) {
+ mutex_unlock(&rng_mutex);
unregister_miscdev();
if (hwrng_fill)
kthread_stop(hwrng_fill);
- }
-
- mutex_unlock(&rng_mutex);
+ } else
+ mutex_unlock(&rng_mutex);
}
EXPORT_SYMBOL_GPL(hwrng_unregister);
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 6/6] hw_random: don't init list element we're about to add to list.
From: Rusty Russell
Another interesting anti-pattern.
Signed-off-by: Rusty Russell
---
drivers/char/hw_random/core.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index fc5de7d..b2cc8a1 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -493,7 +493,6 @@ int hwrng_register(struct hwrng *rng)
goto out_unlock;
}
}
- INIT_LIST_HEAD(&rng->list);
list_add_tail(&rng->list, &rng_list);
if (old_rng && !rng->init) {
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 0/6] fix hw_random stuck
When I hotunplug a busy virtio-rng device or try to access hwrng attributes in non-smp guest, it gets stuck. My hotplug tests: | test 0: | hotunplug rng device from qemu monitor | | test 1: | guest) # dd if=/dev/hwrng of=/dev/null & | hotunplug rng device from qemu monitor | | test 2: | guest) # dd if=/dev/random of=/dev/null & | hotunplug rng device from qemu monitor | | test 4: | guest) # dd if=/dev/hwrng of=/dev/null & | cat /sys/devices/virtual/misc/hw_random/rng_* | | test 5: | 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 | | test 6: | use a fifo as rng backend, execute test 0 ~ 5 with no input of fifo V4: update patch 4 to fix corrupt, decrease last reference for triggering the cleanup, fix unregister race pointed by Herbert V3: initialize kref to 1 V2: added patch 2 to fix a deadlock, update current patch 3 to fix reference counting issue Amos Kong (1): hw_random: move some code out mutex_lock for avoiding underlying deadlock Rusty Russell (5): hw_random: place mutex around read functions and buffers. hw_random: use reference counts on each struct hwrng. hw_random: fix unregister race. hw_random: don't double-check old_rng. hw_random: don't init list element we're about to add to list. drivers/char/hw_random/core.c | 176 ++ include/linux/hw_random.h | 3 + 2 files changed, 129 insertions(+), 50 deletions(-) -- 1.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 3/6] hw_random: use reference counts on each struct hwrng.
From: Rusty Russell
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.
v4: decrease last reference for triggering the cleanup
v3: initialize kref (thanks Amos Kong)
v2: fix missing put_rng() on exit path (thanks Amos Kong)
Signed-off-by: Rusty Russell
Signed-off-by: Amos Kong
---
drivers/char/hw_random/core.c | 142 +-
include/linux/hw_random.h | 2 +
2 files changed, 101 insertions(+), 43 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index a0905c8..27ad6b4 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,65 @@ 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)
+{
+ struct hwrng *rng = current_rng;
+
+ BUG_ON(!mutex_is_locked(&rng_mutex));
+ if (!current_rng)
+ return;
+
+ /* release current_rng reference */
+ kref_put(¤t_rng->ref, cleanup_rng);
+ current_rng = NULL;
+
+ /* decrease last reference for triggering the cleanup */
+ kref_put(&rng->ref, cleanup_rng);
+}
+
+/* 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) {
@@ -110,13 +170,9 @@ static inline int hwrng_init(struct hwrng *rng)
if (current_quality > 0 && !hwrng_fill)
start_khwrngd();
- return 0;
-}
+ kref_init(&rng->ref);
-static inline void hwrng_cleanup(struct hwrng *rng)
-{
- if (rng && rng->cleanup)
- rng->cleanup(rng);
+ return 0;
}
static int rng_dev_open(struct inode *inode, struct file *filp)
@@ -154,21 +210,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,8 +257,8 @@ static ssize_t rng_dev_read(struct file *filp, char __user
*buf,
ret += len;
}
- mutex_unlock(&rng_mutex);
mutex_unlock(&reading_mutex);
+ put_rng(rng);
if (need_resched())
schedule_timeout_interruptible(1);
@@ -213,12 +270,11 @@ static ssize_t rng_dev_read(struct file *filp, char
__user *buf,
}
out:
return ret ? : err;
-out_unlock:
- mutex_unlock(&rng_mutex);
- goto out;
+
out_unlock
Re: [PATCH 3/5] hw_random: fix unregister race.
On Tue, Oct 21, 2014 at 10:15:23PM +0800, Herbert Xu wrote: > On Thu, Sep 18, 2014 at 12:18:24PM +0930, Rusty Russell wrote: > > The previous patch added one potential problem: we can still be > > reading from a hwrng when it's unregistered. Add a wait for zero > > in the hwrng_unregister path. > > > > Signed-off-by: Rusty Russell > > --- > > drivers/char/hw_random/core.c | 5 + > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c > > index dc9092a1075d..b4a21e9521cf 100644 > > --- a/drivers/char/hw_random/core.c > > +++ b/drivers/char/hw_random/core.c > > @@ -60,6 +60,7 @@ static DEFINE_MUTEX(rng_mutex); > > static DEFINE_MUTEX(reading_mutex); > > static int data_avail; > > static u8 *rng_buffer, *rng_fillbuf; > > +static DECLARE_WAIT_QUEUE_HEAD(rng_done); > > static unsigned short current_quality; > > static unsigned short default_quality; /* = 0; default to "off" */ > > > > @@ -98,6 +99,7 @@ static inline void cleanup_rng(struct kref *kref) > > > > if (rng->cleanup) > > rng->cleanup(rng); rng->cleanup_done = true; > > + wake_up_all(&rng_done); > > } > > > > static void set_current_rng(struct hwrng *rng) > > @@ -529,6 +531,9 @@ void hwrng_unregister(struct hwrng *rng) > > } > > > > mutex_unlock(&rng_mutex); > > + > > + /* Just in case rng is reading right now, wait. */ > > + wait_event(rng_done, atomic_read(&rng->ref.refcount) == 0); Hi Rusty, After initializing (kref_init()), the refcount is 1, so we need one more kref_put() after each drop_current_rng() to release last reference count, then cleanup function will be called. > While it's obviously better than what we have now, I don't believe > this is 100% safe as the cleanup function might still be running > even after the ref count hits zero. Once we return from this function > the module may be unloaded so we need to ensure that nothing is > running at this point. I found wait_event() can still pass and finish unregister even cleanup function isn't called (wake_up_all() isn't called). So I added a flag cleanup_done to indicate that the rng device is cleaned up. + /* Just in case rng is reading right now, wait. */ + wait_event(rng_done, rng->cleanup_done && + atomic_read(&rng->ref.refcount) == 0); I will post the new v4 later. > Cheers, > -- > Email: Herbert Xu > Home Page: http://gondor.apana.org.au/~herbert/ > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- Amos. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 4/6] hw_random: fix unregister race.
On Sun, Nov 02, 2014 at 11:08:09PM +0800, Herbert Xu wrote: > On Sun, Nov 02, 2014 at 11:06:13PM +0800, Amos Kong wrote: > > On Fri, Oct 31, 2014 at 03:23:21PM +0800, Herbert Xu wrote: > > > On Fri, Oct 31, 2014 at 10:28:00AM +1030, Rusty Russell wrote: > > > > Herbert Xu writes: > > > > > On Thu, Sep 18, 2014 at 08:37:45PM +0800, Amos Kong wrote: > > > > >> From: Rusty Russell > > > > >> > > > > >> The previous patch added one potential problem: we can still be > > > > >> reading from a hwrng when it's unregistered. Add a wait for zero > > > > >> in the hwrng_unregister path. > > > > >> > > > > >> Signed-off-by: Rusty Russell > > > > > > > > > > You totally corrupted Rusty's patch. If you're going to repost > > > > > his series you better make sure that you've got the right patches. > > > > > > > > > > Just as well though as it made me think a little more about this > > > > > patch :) > > > > > > > > OK Amos, can you please repost the complete series? > > > > > > Please fix the unregister race I identified first. > > > > Ok, I redownload the patches from > > https://patchwork.kernel.org/project/LKML/list/?submitter=5 > > and rebase fixes of me and rusty on it. I will post a V4 later. Thanks. > > Please fix the unregister race I pointed out before doing a v4. Thanks for the remind, I got it right now :-) -- Amos. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 4/6] hw_random: fix unregister race.
On Fri, Oct 31, 2014 at 03:23:21PM +0800, Herbert Xu wrote: > On Fri, Oct 31, 2014 at 10:28:00AM +1030, Rusty Russell wrote: > > Herbert Xu writes: > > > On Thu, Sep 18, 2014 at 08:37:45PM +0800, Amos Kong wrote: > > >> From: Rusty Russell > > >> > > >> The previous patch added one potential problem: we can still be > > >> reading from a hwrng when it's unregistered. Add a wait for zero > > >> in the hwrng_unregister path. > > >> > > >> Signed-off-by: Rusty Russell > > > > > > You totally corrupted Rusty's patch. If you're going to repost > > > his series you better make sure that you've got the right patches. > > > > > > Just as well though as it made me think a little more about this > > > patch :) > > > > OK Amos, can you please repost the complete series? > > Please fix the unregister race I identified first. Ok, I redownload the patches from https://patchwork.kernel.org/project/LKML/list/?submitter=5 and rebase fixes of me and rusty on it. I will post a V4 later. Thanks. -- Amos. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 3/6] hw_random: use reference counts on each struct hwrng.
On Mon, Oct 20, 2014 at 10:42:11AM +1030, Rusty Russell wrote: > Amos Kong writes: > > We got a warning in boot stage when above set_current_rng() is executed, > > it can be fixed by init rng->ref in hwrng_init(). > > > > > > @@ -166,6 +169,8 @@ static inline int hwrng_init(struct hwrng *rng) > > if (current_quality > 0 && !hwrng_fill) > > start_khwrngd(); > > > > + kref_init(&rng->ref); > > + > > return 0; > > } > > OK, I folded this fix on. Thanks. Reviewed-by: Amos Kong > Thanks, > Rusty. > > 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. > > v3: initialize kref (thanks Amos Kong) > v2: fix missing put_rng() on exit path (thanks Amos Kong) > Signed-off-by: Rusty Russell > > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c > index a0905c818e13..0ecac38da954 100644 > --- a/drivers/char/hw_random/core.c > +++ b/drivers/char/hw_random/core.c -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[3.16 stable PATCH 1/2] virtio-rng: fix stuck of hot-unplugging busy device
When we try to hot-remove a busy virtio-rng device from QEMU monitor,
the device can't be hot-removed. Because virtio-rng driver hangs at
wait_for_completion_killable().
This patch exits the waiting by completing have_data completion before
unregistering, resets data_avail to avoid the hwrng core use wrong
buffer bytes.
Signed-off-by: Amos Kong
Reviewed-by: Amit Shah
Cc: sta...@vger.kernel.org
Signed-off-by: Rusty Russell
(cherry picked from commit 3856e548372513665670ca5db60d9a74b970fe0d)
Signed-off-by: Amos Kong
---
drivers/char/hw_random/virtio-rng.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index f1aa13b..b50252c 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -137,6 +137,8 @@ static void remove_common(struct virtio_device *vdev)
{
struct virtrng_info *vi = vdev->priv;
+ vi->data_avail = 0;
+ complete(&vi->have_data);
vdev->config->reset(vdev);
vi->busy = false;
if (vi->hwrng_register_done)
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[3.16 stable PATCH 0/2] virtio-rng: two backports to fix stuck
I received two mails about faile to apply patches to 3.16-stable tree: FAILED: patch "[PATCH] virtio-rng: skip reading when we start to remove the device" failed to apply to 3.16-stable tree FAILED: patch "[PATCH] virtio-rng: fix stuck of hot-unplugging busy device" failed to apply to 3.16-stable tree Amit already backported two patches for 3.16-stable, then cherry-pick of my two patches works. Thanks. Amos Kong (2): virtio-rng: fix stuck of hot-unplugging busy device virtio-rng: skip reading when we start to remove the device drivers/char/hw_random/virtio-rng.c | 7 +++ 1 file changed, 7 insertions(+) -- 1.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[3.16 stable PATCH 2/2] virtio-rng: skip reading when we start to remove the device
Before we really unregister the hwrng device, reading will get stuck if
the virtio device is reset. We should return error for reading when we
start to remove the device.
Signed-off-by: Amos Kong
Reviewed-by: Amit Shah
Cc: sta...@vger.kernel.org
Signed-off-by: Rusty Russell
(cherry picked from commit f49819560f53b7f3a596a8ea2e6764dc86695b62)
Signed-off-by: Amos Kong
---
drivers/char/hw_random/virtio-rng.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/char/hw_random/virtio-rng.c
b/drivers/char/hw_random/virtio-rng.c
index b50252c..cb1688a 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -37,6 +37,7 @@ struct virtrng_info {
char name[25];
int index;
bool hwrng_register_done;
+ bool hwrng_removed;
};
@@ -69,6 +70,9 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t
size, bool wait)
int ret;
struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
+ if (vi->hwrng_removed)
+ return -ENODEV;
+
if (!vi->busy) {
vi->busy = true;
init_completion(&vi->have_data);
@@ -137,6 +141,7 @@ static void remove_common(struct virtio_device *vdev)
{
struct virtrng_info *vi = vdev->priv;
+ vi->hwrng_removed = true;
vi->data_avail = 0;
complete(&vi->have_data);
vdev->config->reset(vdev);
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 3/6] hw_random: use reference counts on each struct hwrng.
On Thu, Sep 18, 2014 at 08:37:44PM +0800, Amos Kong wrote:
> From: Rusty Russell
>
> 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.
>
> V2: reduce reference count in signal_pending path
>
> Signed-off-by: Rusty Russell
> Signed-off-by: Amos Kong
Reply myself.
> ---
> drivers/char/hw_random/core.c | 136
> +-
> include/linux/hw_random.h | 2 +
> 2 files changed, 95 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index a0905c8..ee9e504 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(&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(¤t_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_resche
Re: [PATCH v2 2/3] hw_random: fix stuck in catting hwrng attributes
On Thu, Sep 18, 2014 at 12:13:08PM +0930, Rusty Russell wrote:
> Amos Kong writes:
>
> > I started a QEMU (non-smp) guest with one virtio-rng device, and read
> > random data from /dev/hwrng by dd:
> >
> > # dd if=/dev/hwrng of=/dev/null &
> >
> > In the same time, if I check hwrng attributes from sysfs by cat:
> >
> > # cat /sys/class/misc/hw_random/rng_*
> >
> > The cat process always gets stuck with slow backend (5 k/s), if we
> > use a quick backend (1.2 M/s), the cat process will cost 1 to 2
> > minutes. The stuck doesn't exist for smp guest.
> >
> > Reading syscall enters kernel and call rng_dev_read(), it's user
> > context. We used need_resched() to check if other tasks need to
> > be run, but it almost always return false, and re-hold the mutex
> > lock. The attributes accessing process always fails to hold the
> > lock, so the cat gets stuck.
> >
> > User context doesn't allow other user contexts run on that CPU,
> > unless the kernel code sleeps for some reason. This is why the
> > need_reshed() always return false here.
> >
> > This patch removed need_resched() and always schedule other tasks
> > then other tasks can have chance to hold the lock and execute
> > protected code.
Hi Rusty,
> OK, this is going to be a rant.
>
> Your explanation doesn't make sense at all. Worse, your solution breaks
> the advice of Kernighan & Plaugher: "Don't patch bad code - rewrite
> it.".
>
> But worst of all, this detailed explanation might have convinced me you
> understood the problem better than I did, and applied your patch.
I'm sorry about the misleading.
> I did some tests. For me, as expected, the process spends its time
> inside the virtio rng read function, holding the mutex and thus blocking
> sysfs access; it's not a failure of this code at all.
Got it now.
The catting hang bug was found when I try to fix unhotplug issue, the
unhotplug issue can't be reproduced if I try to debug by gdb or
printk. So I forgot to debug cat hang ... but spend time to misunderstand
schedle code :(
> Your schedule_timeout() "fix" probably just helps by letting the host
> refresh entropy, so we spend less time waiting in the read fn.
>
> I will post a series, which unfortunately is only lightly tested, then
> I'm going to have some beer to begin my holiday. That may help me
> forget my disappointment at seeing respected fellow developers
> monkey-patching random code they don't understand.
I just posted a V2 with two additional fixes, hotunplugging works well now :)
> Grrr
Enjoy your holiday!
Amos
> Rusty.
>
> > Signed-off-by: Amos Kong
> > ---
> > drivers/char/hw_random/core.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> > index c591d7e..263a370 100644
> > --- a/drivers/char/hw_random/core.c
> > +++ b/drivers/char/hw_random/core.c
> > @@ -195,8 +195,7 @@ static ssize_t rng_dev_read(struct file *filp, char
> > __user *buf,
> >
> > mutex_unlock(&rng_mutex);
> >
> > - if (need_resched())
> > - schedule_timeout_interruptible(1);
> > + schedule_timeout_interruptible(1);
> >
> > if (signal_pending(current)) {
> > err = -ERESTARTSYS;
> > --
> > 1.9.3
--
Amos.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 1/6] hw_random: place mutex around read functions and buffers.
From: Rusty Russell
There's currently a big lock around everything, and it means that we
can't query sysfs (eg /sys/devices/virtual/misc/hw_random/rng_current)
while the rng is reading. This is a real problem when the rng is slow,
or blocked (eg. virtio_rng with qemu's default /dev/random backend)
This doesn't help (it leaves the current lock untouched), just adds a
lock to protect the read function and the static buffers, in preparation
for transition.
Signed-off-by: Rusty Russell
---
drivers/char/hw_random/core.c | 20 +---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index aa30a25..b1b6042 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -53,7 +53,10 @@
static struct hwrng *current_rng;
static struct task_struct *hwrng_fill;
static LIST_HEAD(rng_list);
+/* Protects rng_list and current_rng */
static DEFINE_MUTEX(rng_mutex);
+/* Protects rng read functions, data_avail, rng_buffer and rng_fillbuf */
+static DEFINE_MUTEX(reading_mutex);
static int data_avail;
static u8 *rng_buffer, *rng_fillbuf;
static unsigned short current_quality;
@@ -81,7 +84,9 @@ static void add_early_randomness(struct hwrng *rng)
unsigned char bytes[16];
int bytes_read;
+ mutex_lock(&reading_mutex);
bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
+ mutex_unlock(&reading_mutex);
if (bytes_read > 0)
add_device_randomness(bytes, bytes_read);
}
@@ -128,6 +133,7 @@ static inline int rng_get_data(struct hwrng *rng, u8
*buffer, size_t size,
int wait) {
int present;
+ BUG_ON(!mutex_is_locked(&reading_mutex));
if (rng->read)
return rng->read(rng, (void *)buffer, size, wait);
@@ -160,13 +166,14 @@ static ssize_t rng_dev_read(struct file *filp, char
__user *buf,
goto out_unlock;
}
+ mutex_lock(&reading_mutex);
if (!data_avail) {
bytes_read = rng_get_data(current_rng, rng_buffer,
rng_buffer_size(),
!(filp->f_flags & O_NONBLOCK));
if (bytes_read < 0) {
err = bytes_read;
- goto out_unlock;
+ goto out_unlock_reading;
}
data_avail = bytes_read;
}
@@ -174,7 +181,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user
*buf,
if (!data_avail) {
if (filp->f_flags & O_NONBLOCK) {
err = -EAGAIN;
- goto out_unlock;
+ goto out_unlock_reading;
}
} else {
len = data_avail;
@@ -186,7 +193,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user
*buf,
if (copy_to_user(buf + ret, rng_buffer + data_avail,
len)) {
err = -EFAULT;
- goto out_unlock;
+ goto out_unlock_reading;
}
size -= len;
@@ -194,6 +201,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user
*buf,
}
mutex_unlock(&rng_mutex);
+ mutex_unlock(&reading_mutex);
if (need_resched())
schedule_timeout_interruptible(1);
@@ -208,6 +216,9 @@ out:
out_unlock:
mutex_unlock(&rng_mutex);
goto out;
+out_unlock_reading:
+ mutex_unlock(&reading_mutex);
+ goto out_unlock;
}
@@ -348,13 +359,16 @@ static int hwrng_fillfn(void *unused)
while (!kthread_should_stop()) {
if (!current_rng)
break;
+ mutex_lock(&reading_mutex);
rc = rng_get_data(current_rng, rng_fillbuf,
rng_buffer_size(), 1);
+ mutex_unlock(&reading_mutex);
if (rc <= 0) {
pr_warn("hwrng: no data available\n");
msleep_interruptible(1);
continue;
}
+ /* Outside lock, sure, but y'know: randomness. */
add_hwgenerator_randomness((void *)rng_fillbuf, rc,
rc * current_quality * 8 >> 10);
}
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 2/6] hw_random: move some code out mutex_lock for avoiding underlying deadlock
In next patch, we use reference counting for each struct hwrng,
changing reference count also needs to take mutex_lock. Before
releasing the lock, if we try to stop a kthread that waits to
take the lock to reduce the referencing count, deadlock will
occur.
Signed-off-by: Amos Kong
---
drivers/char/hw_random/core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index b1b6042..a0905c8 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -474,12 +474,12 @@ void hwrng_unregister(struct hwrng *rng)
}
}
if (list_empty(&rng_list)) {
+ mutex_unlock(&rng_mutex);
unregister_miscdev();
if (hwrng_fill)
kthread_stop(hwrng_fill);
- }
-
- mutex_unlock(&rng_mutex);
+ } else
+ mutex_unlock(&rng_mutex);
}
EXPORT_SYMBOL_GPL(hwrng_unregister);
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 4/6] hw_random: fix unregister race.
From: Rusty Russell The previous patch added one potential problem: we can still be reading from a hwrng when it's unregistered. Add a wait for zero in the hwrng_unregister path. Signed-off-by: Rusty Russell --- drivers/char/hw_random/core.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c index ee9e504..9f6f5bb 100644 --- a/drivers/char/hw_random/core.c +++ b/drivers/char/hw_random/core.c @@ -60,6 +60,7 @@ static DEFINE_MUTEX(rng_mutex); static DEFINE_MUTEX(reading_mutex); static int data_avail; static u8 *rng_buffer, *rng_fillbuf; +static DECLARE_WAIT_QUEUE_HEAD(rng_done); static unsigned short current_quality; static unsigned short default_quality; /* = 0; default to "off" */ @@ -98,6 +99,7 @@ static inline void cleanup_rng(struct kref *kref) if (rng->cleanup) rng->cleanup(rng); + wake_up_all(&rng_done); } static void set_current_rng(struct hwrng *rng) -- 1.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 0/6] fix hw_random stuck
When I hotunplug a busy virtio-rng device or try to access hwrng attributes in non-smp guest, it gets stuck. My original was pain, Rusty posted a real fix. This patchset fixed two issue in v1, and tested by my 6+ cases. | test 0: | hotunplug rng device from qemu monitor | | test 1: | guest) # dd if=/dev/hwrng of=/dev/null & | hotunplug rng device from qemu monitor | | test 2: | guest) # dd if=/dev/random of=/dev/null & | hotunplug rng device from qemu monitor | | test 4: | guest) # dd if=/dev/hwrng of=/dev/null & | cat /sys/devices/virtual/misc/hw_random/rng_* | | test 5: | 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 | | test 6: | use a fifo as rng backend, execute test 0 ~ 5 with no input of fifo V2: added patch 2 to fix a deadlock, update current patch 3 to fix reference counting issue Amos Kong (1): hw_random: move some code out mutex_lock for avoiding underlying deadlock Rusty Russell (5): hw_random: place mutex around read functions and buffers. hw_random: use reference counts on each struct hwrng. hw_random: fix unregister race. hw_random: don't double-check old_rng. hw_random: don't init list element we're about to add to list. drivers/char/hw_random/core.c | 166 +- include/linux/hw_random.h | 2 + 2 files changed, 117 insertions(+), 51 deletions(-) -- 1.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 5/6] hw_random: don't double-check old_rng.
From: Rusty Russell
Interesting anti-pattern.
Signed-off-by: Rusty Russell
---
drivers/char/hw_random/core.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 9f6f5bb..6420409 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -473,14 +473,13 @@ int hwrng_register(struct hwrng *rng)
}
old_rng = current_rng;
+ err = 0;
if (!old_rng) {
err = hwrng_init(rng);
if (err)
goto out_unlock;
set_current_rng(rng);
- }
- err = 0;
- if (!old_rng) {
+
err = register_miscdev();
if (err) {
drop_current_rng();
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 3/6] hw_random: use reference counts on each struct hwrng.
From: Rusty Russell
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.
V2: reduce reference count in signal_pending path
Signed-off-by: Rusty Russell
Signed-off-by: Amos Kong
---
drivers/char/hw_random/core.c | 136 +-
include/linux/hw_random.h | 2 +
2 files changed, 95 insertions(+), 43 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index a0905c8..ee9e504 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(&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(¤t_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())
@@ -208,17 +256,19 @@ static ssize_t rng_dev_read(struct file *filp, char
__user *buf,
if (signal_pending(current)) {
err = -ERESTARTSYS;
+ put_rng(rng);
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 +307,8 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
err = hwrng_init(rng);
[PATCH v2 6/6] hw_random: don't init list element we're about to add to list.
From: Rusty Russell
Another interesting anti-pattern.
Signed-off-by: Rusty Russell
---
drivers/char/hw_random/core.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 6420409..12187dd 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -486,7 +486,6 @@ int hwrng_register(struct hwrng *rng)
goto out_unlock;
}
}
- INIT_LIST_HEAD(&rng->list);
list_add_tail(&rng->list, &rng_list);
if (old_rng && !rng->init) {
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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(&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;
> }
> @@ -35
Re: [PATCH 2/2] virtio-rng: fix stuck in catting hwrng attributes
CC linux-kernel Original thread: http://comments.gmane.org/gmane.linux.kernel.virtualization/22775 On Mon, Sep 15, 2014 at 06:48:46PM +0200, Radim Krčmář wrote: > 2014-09-14 10:25+0800, Amos Kong: > > On Sun, Sep 14, 2014 at 09:12:08AM +0800, Amos Kong wrote: > > > > ... > > > > > > diff --git a/drivers/char/hw_random/core.c > > > > > > b/drivers/char/hw_random/core.c > > > > > > index c591d7e..b5d1b6f 100644 > > > > > > --- a/drivers/char/hw_random/core.c > > > > > > +++ b/drivers/char/hw_random/core.c > > > > > > @@ -195,8 +195,7 @@ static ssize_t rng_dev_read(struct file *filp, > > > > > > char __user *buf, > > > > > > > > > > > > mutex_unlock(&rng_mutex); > > > > > > > > > > > > - if (need_resched()) > > > > > > - schedule_timeout_interruptible(1); > > > > > > + schedule_timeout_interruptible(10); > > If cond_resched() does not work, it is a bug elsewehere. Thanks for your reply, Jason also told me the TIF_NEED_RESCHED should be set in this case, then need_resched() returns true. I will investigate the issue and reply you later. > > Problem only occurred in non-smp guest, we can improve it to: > > > > if(!is_smp()) > > schedule_timeout_interruptible(10); > > > > is_smp() is only available for arm arch, we need a general one. > > (It is num_online_cpus() > 1.) -- Amos. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] virtio-rng cleanup: move some code out of mutex protection
On Mon, Sep 15, 2014 at 06:13:20PM +0200, Michael Büsch wrote:
> On Tue, 16 Sep 2014 00:02:27 +0800
> Amos Kong wrote:
>
> > It doesn't save too much cpu time as expected, just a cleanup.
> >
> > Signed-off-by: Amos Kong
> > ---
> > drivers/char/hw_random/core.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> > index aa30a25..c591d7e 100644
> > --- a/drivers/char/hw_random/core.c
> > +++ b/drivers/char/hw_random/core.c
> > @@ -270,8 +270,8 @@ static ssize_t hwrng_attr_current_show(struct device
> > *dev,
> > return -ERESTARTSYS;
> > if (current_rng)
> > name = current_rng->name;
> > - ret = snprintf(buf, PAGE_SIZE, "%s\n", name);
> > mutex_unlock(&rng_mutex);
> > + ret = snprintf(buf, PAGE_SIZE, "%s\n", name);
>
> I'm not sure this is safe.
> Name is just a pointer.
> What if the hwrng gets unregistered after unlock and just before the snprintf?
Oh, it points to protected current_rng->name, I will drop this
cleanup. Thanks.
> > return ret;
> > }
> > @@ -284,19 +284,19 @@ static ssize_t hwrng_attr_available_show(struct
> > device *dev,
> > ssize_t ret = 0;
> > struct hwrng *rng;
> >
> > + buf[0] = '\0';
> > err = mutex_lock_interruptible(&rng_mutex);
> > if (err)
> > return -ERESTARTSYS;
> > - buf[0] = '\0';
> > list_for_each_entry(rng, &rng_list, list) {
> > strncat(buf, rng->name, PAGE_SIZE - ret - 1);
> > ret += strlen(rng->name);
> > strncat(buf, " ", PAGE_SIZE - ret - 1);
> > ret++;
> > }
> > + mutex_unlock(&rng_mutex);
> > strncat(buf, "\n", PAGE_SIZE - ret - 1);
> > ret++;
> > - mutex_unlock(&rng_mutex);
> >
> > return ret;
> > }
>
> This looks ok.
>
> --
> Michael
--
Amos.
pgpN_tV90Y3_t.pgp
Description: PGP signature
Re: [PATCH v2 3/3] hw_random: increase schedule timeout in rng_dev_read()
On Mon, Sep 15, 2014 at 06:13:31PM +0200, Michael Büsch wrote:
> On Tue, 16 Sep 2014 00:02:29 +0800
> Amos Kong wrote:
>
> > This patch increases the schedule timeout to 10 jiffies, it's more
> > appropriate, then other takes can easy to hold the mutex lock.
> >
> > Signed-off-by: Amos Kong
> > ---
> > drivers/char/hw_random/core.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> > index 263a370..b5d1b6f 100644
> > --- a/drivers/char/hw_random/core.c
> > +++ b/drivers/char/hw_random/core.c
> > @@ -195,7 +195,7 @@ static ssize_t rng_dev_read(struct file *filp, char
> > __user *buf,
> >
> > mutex_unlock(&rng_mutex);
> >
> > - schedule_timeout_interruptible(1);
> > + schedule_timeout_interruptible(10);
> >
> > if (signal_pending(current)) {
> > err = -ERESTARTSYS;
>
> Does a schedule of 1 ms or 10 ms decrease the throughput?
In my test environment, 1 jiffe always works (100%), as suggested by
Amit 10 jiffes is more appropriate.
After applied current 3 patches, there is a throughput regression.
1.2 M/s -> 6 K/s
We can only schedule in the end of loop (size == 0), and only for
non-smp guest. So smp guest won't be effected.
| if (!size && num_online_cpus() == 1)
| schedule_timeout_interruptible(timeout);
Set timeout to 1:
non-smp guest with quick backend (1.2M/s) -> about 49K/s)
Set timeout to 10:
non-smp guest with quick backend (1.2M/s) -> about 490K/s)
We might need other benchmark to test the performance, but we can
see the bug clearly caused a regression.
As we discussed in other thread, need_resched() should work in this
case, so those patches might be wrong fixing.
> I think we need some benchmarks.
>
> --
> Michael
--
Amos.
pgpHIRzKgHNcE.pgp
Description: PGP signature
[PATCH v2 3/3] hw_random: increase schedule timeout in rng_dev_read()
This patch increases the schedule timeout to 10 jiffies, it's more
appropriate, then other takes can easy to hold the mutex lock.
Signed-off-by: Amos Kong
---
drivers/char/hw_random/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 263a370..b5d1b6f 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -195,7 +195,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user
*buf,
mutex_unlock(&rng_mutex);
- schedule_timeout_interruptible(1);
+ schedule_timeout_interruptible(10);
if (signal_pending(current)) {
err = -ERESTARTSYS;
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 1/3] virtio-rng cleanup: move some code out of mutex protection
It doesn't save too much cpu time as expected, just a cleanup.
Signed-off-by: Amos Kong
---
drivers/char/hw_random/core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index aa30a25..c591d7e 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -270,8 +270,8 @@ static ssize_t hwrng_attr_current_show(struct device *dev,
return -ERESTARTSYS;
if (current_rng)
name = current_rng->name;
- ret = snprintf(buf, PAGE_SIZE, "%s\n", name);
mutex_unlock(&rng_mutex);
+ ret = snprintf(buf, PAGE_SIZE, "%s\n", name);
return ret;
}
@@ -284,19 +284,19 @@ static ssize_t hwrng_attr_available_show(struct device
*dev,
ssize_t ret = 0;
struct hwrng *rng;
+ buf[0] = '\0';
err = mutex_lock_interruptible(&rng_mutex);
if (err)
return -ERESTARTSYS;
- buf[0] = '\0';
list_for_each_entry(rng, &rng_list, list) {
strncat(buf, rng->name, PAGE_SIZE - ret - 1);
ret += strlen(rng->name);
strncat(buf, " ", PAGE_SIZE - ret - 1);
ret++;
}
+ mutex_unlock(&rng_mutex);
strncat(buf, "\n", PAGE_SIZE - ret - 1);
ret++;
- mutex_unlock(&rng_mutex);
return ret;
}
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 2/3] hw_random: fix stuck in catting hwrng attributes
I started a QEMU (non-smp) guest with one virtio-rng device, and read
random data from /dev/hwrng by dd:
# dd if=/dev/hwrng of=/dev/null &
In the same time, if I check hwrng attributes from sysfs by cat:
# cat /sys/class/misc/hw_random/rng_*
The cat process always gets stuck with slow backend (5 k/s), if we
use a quick backend (1.2 M/s), the cat process will cost 1 to 2
minutes. The stuck doesn't exist for smp guest.
Reading syscall enters kernel and call rng_dev_read(), it's user
context. We used need_resched() to check if other tasks need to
be run, but it almost always return false, and re-hold the mutex
lock. The attributes accessing process always fails to hold the
lock, so the cat gets stuck.
User context doesn't allow other user contexts run on that CPU,
unless the kernel code sleeps for some reason. This is why the
need_reshed() always return false here.
This patch removed need_resched() and always schedule other tasks
then other tasks can have chance to hold the lock and execute
protected code.
Signed-off-by: Amos Kong
---
drivers/char/hw_random/core.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index c591d7e..263a370 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -195,8 +195,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user
*buf,
mutex_unlock(&rng_mutex);
- if (need_resched())
- schedule_timeout_interruptible(1);
+ schedule_timeout_interruptible(1);
if (signal_pending(current)) {
err = -ERESTARTSYS;
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 0/3] fix stuck in accessing hwrng attributes
If we read hwrng by long-running dd process, it takes too much cpu time and almost hold the mutex lock. When we check hwrng attributes from sysfs by cat, it gets stuck in waiting the lock releaseing. The problem can only be reproduced with non-smp guest with slow backend. This patchset resolves the issue by changing rng_dev_read() to always schedule 10 jiffies after release mutex lock, then cat process can have chance to get the lock and execute protected code without stuck. Thanks. V2: update commitlog to describe PATCH 2, split second patch. Amos Kong (3): virtio-rng cleanup: move some code out of mutex protection hw_random: fix stuck in catting hwrng attributes hw_random: increase schedule timeout in rng_dev_read() drivers/char/hw_random/core.c | 9 - 1 file changed, 4 insertions(+), 5 deletions(-) -- 1.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 1/1] Drivers: hv: vmbus: Enable interrupt driven flow control
On Sat, Sep 6, 2014 at 8:29 AM, K. Y. Srinivasan wrote: > > In win8 we have a feature that allows for interrupt driven flow management > for host/guest communication. For instance, if the host were blocked because > there was no space available in the ringbuffer, the host could request that > the > guest send an interrupt when space becomes available in the ringbuffer (when > the guest drains the ringbuffer). > > While this feature was implemented in the guest a while ago, we had not > advertised that the guest supported this feature. This patch advertises > the support to the host. > > For pre-win8 hosts, this has no effect since the size of the ringbuffer > control structure has not changed and all changes have been backward > compatible - unused/reserved space has been used to implement this > feature. > > In this version of the patch I have cleaned up the commit log based on > feedback from Greg KH. > > Signed-off-by: K. Y. Srinivasan > --- > drivers/hv/ring_buffer.c |5 + > 1 files changed, 5 insertions(+), 0 deletions(-) > > diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c > index 15db66b..6361d12 100644 > --- a/drivers/hv/ring_buffer.c > +++ b/drivers/hv/ring_buffer.c > @@ -361,6 +361,11 @@ int hv_ringbuffer_init(struct hv_ring_buffer_info > *ring_info, > ring_info->ring_buffer->read_index = > ring_info->ring_buffer->write_index = 0; > > + /* > +* Set the feature bit for enabling flow control. > +*/ > + ring_info->ring_buffer->feature_bits.value = 1; > + This feature is enabled, do we need to reset pending_send_sz and reserved1[12], reserved1[4028] here? > ring_info->ring_size = buflen; > ring_info->ring_datasize = buflen - sizeof(struct hv_ring_buffer); > > -- > 1.7.4.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 2/2] net: exit busy loop when another process is runnable
On Thu, Aug 21, 2014 at 04:05:10PM +0800, Jason Wang wrote: > Rx busy loop does not scale well in the case when several parallel > sessions is active. This is because we keep looping even if there's > another process is runnable. For example, if that process is about to > send packet, keep busy polling in current process will brings extra > delay and damage the performance. > > This patch solves this issue by exiting the busy loop when there's > another process is runnable in current cpu. Simple test that pin two > netperf sessions in the same cpu in receiving side shows obvious > improvement: > > Before: > netperf -H 192.168.100.2 -T 0,0 -t TCP_RR -P 0 & \ > netperf -H 192.168.100.2 -T 1,0 -t TCP_RR -P 0 > 16384 87380 11 10.0015513.74 > 16384 87380 > 16384 87380 11 10.0015092.78 > 16384 87380 > > After: > netperf -H 192.168.100.2 -T 0,0 -t TCP_RR -P 0 & \ > netperf -H 192.168.100.2 -T 1,0 -t TCP_RR -P 0 > 16384 87380 11 10.0023334.53 > 16384 87380 > 16384 87380 11 10.0023327.58 > 16384 87380 > > Benchmark was done through two 8 cores Xeon machine back to back connected > with mlx4 through netperf TCP_RR test (busy_read were set to 50): > > sessions/bytes/before/after/+improvement%/busy_read=0/ > 1/1/30062.10/30034.72/+0%/20228.96/ > 16/1/214719.83/307669.01/+43%/268997.71/ > 32/1/231252.81/345845.16/+49%/336157.442/ > 64/512/212467.39/373464.93/+75%/397449.375/ > > Signed-off-by: Jason Wang > --- > include/net/busy_poll.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/include/net/busy_poll.h b/include/net/busy_poll.h > index 1d67fb6..8a33fb2 100644 > --- a/include/net/busy_poll.h > +++ b/include/net/busy_poll.h > @@ -109,7 +109,8 @@ static inline bool sk_busy_loop(struct sock *sk, int > nonblock) > cpu_relax(); > > } while (!nonblock && skb_queue_empty(&sk->sk_receive_queue) && > - !need_resched() && !busy_loop_timeout(end_time)); > + !need_resched() && !busy_loop_timeout(end_time) && > + nr_running_this_cpu() < 2); Reviewed-by: Amos Kong > rc = !skb_queue_empty(&sk->sk_receive_queue); > out: > -- > 1.8.3.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- Amos. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] virtio: rng: add derating factor for use by hwrng core
On Thu, Aug 14, 2014 at 04:39:07AM +0930, Rusty Russell wrote: > The khwrngd thread is started when a hwrng device of sufficient > quality is registered. The virtio-rng device is backed by the > hypervisor, and we trust the hypervisor to provide real entropy. > > A malicious hypervisor is a scenario that's irrelevant -- such a setup > is bound to cause all sorts of badness, and a compromised hwrng is not > the biggest threat. > > Given this, we are certain the quality of randomness we receive is > perfectly trustworthy. Hence, we use 100% for the factor, indicating > maximum confidence in the source. > > Signed-off-by: Amit Shah > Signed-off-by: Rusty Russell Reviewed-by: Amos Kong > --- > Pretty small and contained patch; would be great if it is picked up for > 3.17. > > v2: re-word commit msg > > [Agreed, re-sending to Linus with SOB before jumping on plane] > --- > drivers/char/hw_random/virtio-rng.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/char/hw_random/virtio-rng.c > b/drivers/char/hw_random/virtio-rng.c > index 0027137..2e3139e 100644 > --- a/drivers/char/hw_random/virtio-rng.c > +++ b/drivers/char/hw_random/virtio-rng.c > @@ -116,6 +116,7 @@ static int probe_common(struct virtio_device *vdev) > .cleanup = virtio_cleanup, > .priv = (unsigned long)vi, > .name = vi->name, > + .quality = 1000, > }; > vdev->priv = vi; > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- Amos. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] hv: use correct order when freeing monitor_pages
On Tue, May 27, 2014 at 07:16:20PM +0200, Radim Krčmář wrote: > We try to free two pages when only one has been allocated. > Cleanup path is unlikely, so I haven't found any trace that would fit, > but I hope that free_pages_prepare() does catch it. > > Cc: sta...@vger.kernel.org > Signed-off-by: Radim Krčmář > --- > Cc'd stable because the worst-case looks hard to debug. > Btw. the module can't get unloaded after we successfully connect? > > drivers/hv/connection.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c > index 7f10c15..e84f452 100644 > --- a/drivers/hv/connection.c > +++ b/drivers/hv/connection.c > @@ -224,8 +224,8 @@ cleanup: > vmbus_connection.int_page = NULL; > } > > - free_pages((unsigned long)vmbus_connection.monitor_pages[0], 1); > - free_pages((unsigned long)vmbus_connection.monitor_pages[1], 1); > + free_pages((unsigned long)vmbus_connection.monitor_pages[0], 0); > + free_pages((unsigned long)vmbus_connection.monitor_pages[1], 0); Allocate order is 0. (2^0 = 1 page) vmbus_connection.monitor_pages[0] = (void *)__get_free_pages((GFP_KERNEL|__GFP_ZERO), 0); vmbus_connection.monitor_pages[1] = (void *)__get_free_pages((GFP_KERNEL|__GFP_ZERO), 0); Looks good. Reviewed-by: Amos Kong > vmbus_connection.monitor_pages[0] = NULL; > vmbus_connection.monitor_pages[1] = NULL; > > -- > 1.9.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- Amos. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drm/qxl: return IRQ_NONE if it was not our irq
On Mon, May 12, 2014 at 04:35:39PM +0800, Jason Wang wrote:
> Return IRQ_NONE if it was not our irq. This is necessary for the case
> when qxl is sharing irq line with a device A in a crash kernel. If qxl
> is initialized before A and A's irq was raised during this gap,
> returning IRQ_HANDLED in this case will cause this irq to be raised
> again after EOI since kernel think it was handled but in fact it was
> not.
>
> Cc: Gerd Hoffmann
> Cc: sta...@vger.kernel.org
> Signed-off-by: Jason Wang
> ---
> drivers/gpu/drm/qxl/qxl_irq.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_irq.c b/drivers/gpu/drm/qxl/qxl_irq.c
> index 28f84b4..3485bdc 100644
> --- a/drivers/gpu/drm/qxl/qxl_irq.c
> +++ b/drivers/gpu/drm/qxl/qxl_irq.c
> @@ -33,6 +33,9 @@ irqreturn_t qxl_irq_handler(int irq, void *arg)
>
> pending = xchg(&qdev->ram_header->int_pending, 0);
>
> + if (!pending)
> + return IRQ_NONE;
> +
Looks correct.
Revewed-by: Amos Kong
> atomic_inc(&qdev->irq_received);
>
> if (pending & QXL_INTERRUPT_DISPLAY) {
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Amos.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] macvtap: fix up direction in comment on offloading
On Fri, Aug 16, 2013 at 1:46 AM, Michael S. Tsirkin wrote: > It speaks about receiving frames, so while > it says GSO, it really means GRO. > > Signed-off-by: Michael S. Tsirkin > --- > drivers/net/macvtap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c > index a98fb0e..a98ed9f 100644 > --- a/drivers/net/macvtap.c > +++ b/drivers/net/macvtap.c > @@ -1047,7 +1047,7 @@ static int set_offload(struct macvtap_queue *q, > unsigned long arg) > * accept TSO frames and turning it off means that user space > * does not support TSO. > * For macvtap, we have to invert it to mean the same thing. > -* When user space turns off TSO, we turn off GSO/LRO so that > +* When user space turns off TSO, we turn off GRO/LRO so that Right fix. Reviewed-by: Amos Kong > * user-space will not receive TSO frames. > */ > if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO)) > -- > MST > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] thinkpad_acpi: return -NODEV while operating uninitialized LEDs
On Fri, Jun 7, 2013 at 5:05 PM, Adam Lee wrote:
>
> Not all 0-15 LEDs are available for all models, sometimes it's even not
> safe. This patch return -NODEV while operating uninitialized LEDs.
>
> Signed-off-by: Adam Lee
> ---
> drivers/platform/x86/thinkpad_acpi.c |5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index 54d31c0..d2ac4e8 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -5401,9 +5401,12 @@ static int led_write(char *buf)
> return -ENODEV;
>
> while ((cmd = next_cmd(&buf))) {
> - if (sscanf(cmd, "%d", &led) != 1 || led < 0 || led > 15)
> + if (sscanf(cmd, "%d", &led) != 1 || led < 0 || led >
> (TPACPI_LED_NUMLEDS -1))
> return -EINVAL;
>
> + if (!tpacpi_leds[led].led)
The .led filed might be set to '0' in led_init(), how about set the
uninitiated .led to -1? and check by "if (tpacpi_leds[led].led < 0)"
for (i = 0; i < TPACPI_LED_NUMLEDS; i++) {
if (!tpacpi_is_led_restricted(i) &&
test_bit(i, &useful_leds)) {
rc = tpacpi_init_led(i);
if (rc < 0) {
led_exit();
return rc;
- }
+ } else
+ tpacpi_leds[led].led = -1;
} else tpacpi_leds[led].led = -1;
}
> + return -ENODEV;
> +
> if (strstr(cmd, "off")) {
> s = TPACPI_LED_OFF;
> } else if (strstr(cmd, "on")) {
> --
> 1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
