2015-02-03 11:55:53

by Wang Long

[permalink] [raw]
Subject: [PATCH] samples: Fix `echo 1 > /proc/int-fifo` never return error

With the kernel module *samples/kfifo/inttype-example.ko*, the system will
create file /proc/int-fifo.

but the current code for this module may have some bug, as the following:

# echo 100 > /proc/int-fifo ----------------> OK
# echo 1000000 > /proc/int-fifo ----------------> OK
# echo 99 > /proc/int-fifo ----------------> Never return
# echo 1000 > /proc/int-fifo ----------------> Never return

so i found that, the length of the content putting into /proc/int-fifo must be 4, 8, 12 .....

another idea:
this kernel module is about a FIFO which type is int. we should put an interger into it, not the string.
when we reading from this file, we should get all elements in the FIFO. just as the following:

# echo 1 > /proc/int-fifo
# echo 2 > /proc/int-fifo
# echo 3 > /proc/int-fifo
# cat /proc/int-fifo
1
2
3
# echo 101 > /proc/int-fifo
# echo 103 > /proc/int-fifo
# echo 104 > /proc/int-fifo
# echo 102 > /proc/int-fifo
# cat /proc/int-fifo
101
103
104
102

with my patch, we can echo an integer into the FIFO. so the following command is OK.
echo 1 > /proc/int-fifo

without my patch, the command `echo 1 > /proc/int-fifo` will never return, beacuse the following code:

223 int __kfifo_from_user(struct __kfifo *fifo, const void __user *from,
224 unsigned long len, unsigned int *copied)
225 {
226 unsigned int l;
227 unsigned long ret;
228 unsigned int esize = fifo->esize;
229 int err;
230
231 if (esize != 1)
232 len /= esize;
233
234 l = kfifo_unused(fifo);
235 if (len > l)
236 len = l;
237
238 ret = kfifo_copy_from_user(fifo, from, len, fifo->in, copied);
239 if (unlikely(ret)) {
240 len -= ret;
241 err = -EFAULT;
242 } else
243 err = 0;
244 fifo->in += len;
245 return err;
246 }
247 EXPORT_SYMBOL(__kfifo_from_user);

int the line 231: if len = 3, esize = 4, then at the line 232 len = 0.





Wang Long (1):
samples: Fix `echo 1 > /proc/int-fifo` never return error

samples/kfifo/inttype-example.c | 51 ++++++++++++++++++++++++++++++++---------
1 file changed, 40 insertions(+), 11 deletions(-)

--
1.8.3.1


2015-02-03 11:55:49

by Wang Long

[permalink] [raw]
Subject: [PATCH] samples: Fix `echo 1 > /proc/int-fifo` never return error

echo 99 > /proc/int-fifo ------------> Never return
echo 1000 > /proc/int-fifo ------------> Never return

this patch fix it.

Signed-off-by: Wang Long <[email protected]>
---
samples/kfifo/inttype-example.c | 51 ++++++++++++++++++++++++++++++++---------
1 file changed, 40 insertions(+), 11 deletions(-)

diff --git a/samples/kfifo/inttype-example.c b/samples/kfifo/inttype-example.c
index 8dc3c2e..cc0db5f 100644
--- a/samples/kfifo/inttype-example.c
+++ b/samples/kfifo/inttype-example.c
@@ -6,6 +6,7 @@
* Released under the GPL version 2 only.
*
*/
+#include <asm/uaccess.h>

#include <linux/init.h>
#include <linux/module.h>
@@ -23,6 +24,9 @@
/* name of the proc entry */
#define PROC_FIFO "int-fifo"

+/* Worst case buffer size needed for holding an integer. */
+#define PROC_NUMBUF 13
+
/* lock for procfs read access */
static DEFINE_MUTEX(read_lock);

@@ -108,33 +112,58 @@ static int __init testfunc(void)
static ssize_t fifo_write(struct file *file, const char __user *buf,
size_t count, loff_t *ppos)
{
- int ret;
- unsigned int copied;
+ char buffer[PROC_NUMBUF];
+ int value;
+ int err;

- if (mutex_lock_interruptible(&write_lock))
- return -ERESTARTSYS;
+ memset(buffer, 0, sizeof(buffer));

- ret = kfifo_from_user(&test, buf, count, &copied);
+ if (count > sizeof(buffer) - 1)
+ count = sizeof(buffer) - 1;
+ if (copy_from_user(buffer, buf, count)) {
+ err = -EFAULT;
+ goto out;
+ }

- mutex_unlock(&write_lock);
+ err = kstrtoint(strstrip(buffer), 0, &value);
+ if (err)
+ goto out;
+
+ if (kfifo_is_full(&test)) {
+ err = -EINVAL;
+ goto out;
+ }

- return ret ? ret : copied;
+ if (mutex_lock_interruptible(&write_lock))
+ return -ERESTARTSYS;
+ kfifo_put(&test, value);
+ mutex_unlock(&write_lock);
+out:
+ return err < 0 ? err : count;
}

static ssize_t fifo_read(struct file *file, char __user *buf,
size_t count, loff_t *ppos)
{
- int ret;
- unsigned int copied;
+ char buffer[PROC_NUMBUF * FIFO_SIZE];
+ int value;
+ size_t len = 0;
+ ssize_t ret = -1;
+
+ memset(buffer, 0, sizeof(buffer));

if (mutex_lock_interruptible(&read_lock))
return -ERESTARTSYS;

- ret = kfifo_to_user(&test, buf, count, &copied);
+ while (!kfifo_is_empty(&test)){
+ ret = kfifo_get(&test, &value);
+ len = snprintf(buffer, sizeof(buffer), "%s%d\n", buffer, value);
+ }

mutex_unlock(&read_lock);
+ ret = copy_to_user(buf, buffer, len);

- return ret ? ret : copied;
+ return ret ? ret : len;
}

static const struct file_operations fifo_fops = {
--
1.8.3.1

2015-02-04 10:36:15

by Stefani Seibold

[permalink] [raw]
Subject: Re: [PATCH] samples: Fix `echo 1 > /proc/int-fifo` never return error

The example is intended for int types, not for strings. So it is not a
bug, it's a feature ;-) But anyway, if you prefer to handle with strings
your are okay by me.

Am Dienstag, den 03.02.2015, 11:51 +0000 schrieb Wang Long:
> echo 99 > /proc/int-fifo ------------> Never return
> echo 1000 > /proc/int-fifo ------------> Never return
>
> this patch fix it.
>
> Signed-off-by: Wang Long <[email protected]>
> ---
> samples/kfifo/inttype-example.c | 51 ++++++++++++++++++++++++++++++++---------
> 1 file changed, 40 insertions(+), 11 deletions(-)
>
> diff --git a/samples/kfifo/inttype-example.c b/samples/kfifo/inttype-example.c
> index 8dc3c2e..cc0db5f 100644
> --- a/samples/kfifo/inttype-example.c
> +++ b/samples/kfifo/inttype-example.c
> @@ -6,6 +6,7 @@
> * Released under the GPL version 2 only.
> *
> */
> +#include <asm/uaccess.h>
>
> #include <linux/init.h>
> #include <linux/module.h>
> @@ -23,6 +24,9 @@
> /* name of the proc entry */
> #define PROC_FIFO "int-fifo"
>
> +/* Worst case buffer size needed for holding an integer. */
> +#define PROC_NUMBUF 13
> +
> /* lock for procfs read access */
> static DEFINE_MUTEX(read_lock);
>
> @@ -108,33 +112,58 @@ static int __init testfunc(void)
> static ssize_t fifo_write(struct file *file, const char __user *buf,
> size_t count, loff_t *ppos)
> {
> - int ret;
> - unsigned int copied;
> + char buffer[PROC_NUMBUF];
> + int value;
> + int err;
>
> - if (mutex_lock_interruptible(&write_lock))
> - return -ERESTARTSYS;
> + memset(buffer, 0, sizeof(buffer));
>
> - ret = kfifo_from_user(&test, buf, count, &copied);
> + if (count > sizeof(buffer) - 1)
> + count = sizeof(buffer) - 1;
> + if (copy_from_user(buffer, buf, count)) {
> + err = -EFAULT;
> + goto out;
> + }
>
> - mutex_unlock(&write_lock);
> + err = kstrtoint(strstrip(buffer), 0, &value);
> + if (err)
> + goto out;
> +
> + if (kfifo_is_full(&test)) {
> + err = -EINVAL;
> + goto out;
> + }
>
> - return ret ? ret : copied;
> + if (mutex_lock_interruptible(&write_lock))
> + return -ERESTARTSYS;
> + kfifo_put(&test, value);
> + mutex_unlock(&write_lock);
> +out:
> + return err < 0 ? err : count;
> }
>
> static ssize_t fifo_read(struct file *file, char __user *buf,
> size_t count, loff_t *ppos)
> {
> - int ret;
> - unsigned int copied;
> + char buffer[PROC_NUMBUF * FIFO_SIZE];
> + int value;
> + size_t len = 0;
> + ssize_t ret = -1;
> +
> + memset(buffer, 0, sizeof(buffer));
>
> if (mutex_lock_interruptible(&read_lock))
> return -ERESTARTSYS;
>
> - ret = kfifo_to_user(&test, buf, count, &copied);
> + while (!kfifo_is_empty(&test)){
> + ret = kfifo_get(&test, &value);
> + len = snprintf(buffer, sizeof(buffer), "%s%d\n", buffer, value);
> + }
>
> mutex_unlock(&read_lock);
> + ret = copy_to_user(buf, buffer, len);
>
> - return ret ? ret : copied;
> + return ret ? ret : len;
> }
>
> static const struct file_operations fifo_fops = {

2015-02-04 10:36:10

by Wang Long

[permalink] [raw]
Subject: Re: [PATCH] samples: Fix `echo 1 > /proc/int-fifo` never return error

On 2015/2/4 18:18, Stefani Seibold wrote:
> The example is intended for int types, not for strings. So it is not a
> bug, it's a feature ;-) But anyway, if you prefer to handle with strings
> your are okay by me.
>
hi,Stefani Seibold

Thanks you for your reply. another question?

This example export the interface '/proc/int-fifo', and how to use it?
Write and read it through system call, or the command `cat` and `echo`?

Best Regards
Wang Long

> Am Dienstag, den 03.02.2015, 11:51 +0000 schrieb Wang Long:
>> echo 99 > /proc/int-fifo ------------> Never return
>> echo 1000 > /proc/int-fifo ------------> Never return
>>
>> this patch fix it.
>>
>> Signed-off-by: Wang Long <[email protected]>
>> ---
>> samples/kfifo/inttype-example.c | 51 ++++++++++++++++++++++++++++++++---------
>> 1 file changed, 40 insertions(+), 11 deletions(-)
>>
>> diff --git a/samples/kfifo/inttype-example.c b/samples/kfifo/inttype-example.c
>> index 8dc3c2e..cc0db5f 100644
>> --- a/samples/kfifo/inttype-example.c
>> +++ b/samples/kfifo/inttype-example.c
>> @@ -6,6 +6,7 @@
>> * Released under the GPL version 2 only.
>> *
>> */
>> +#include <asm/uaccess.h>
>>
>> #include <linux/init.h>
>> #include <linux/module.h>
>> @@ -23,6 +24,9 @@
>> /* name of the proc entry */
>> #define PROC_FIFO "int-fifo"
>>
>> +/* Worst case buffer size needed for holding an integer. */
>> +#define PROC_NUMBUF 13
>> +
>> /* lock for procfs read access */
>> static DEFINE_MUTEX(read_lock);
>>
>> @@ -108,33 +112,58 @@ static int __init testfunc(void)
>> static ssize_t fifo_write(struct file *file, const char __user *buf,
>> size_t count, loff_t *ppos)
>> {
>> - int ret;
>> - unsigned int copied;
>> + char buffer[PROC_NUMBUF];
>> + int value;
>> + int err;
>>
>> - if (mutex_lock_interruptible(&write_lock))
>> - return -ERESTARTSYS;
>> + memset(buffer, 0, sizeof(buffer));
>>
>> - ret = kfifo_from_user(&test, buf, count, &copied);
>> + if (count > sizeof(buffer) - 1)
>> + count = sizeof(buffer) - 1;
>> + if (copy_from_user(buffer, buf, count)) {
>> + err = -EFAULT;
>> + goto out;
>> + }
>>
>> - mutex_unlock(&write_lock);
>> + err = kstrtoint(strstrip(buffer), 0, &value);
>> + if (err)
>> + goto out;
>> +
>> + if (kfifo_is_full(&test)) {
>> + err = -EINVAL;
>> + goto out;
>> + }
>>
>> - return ret ? ret : copied;
>> + if (mutex_lock_interruptible(&write_lock))
>> + return -ERESTARTSYS;
>> + kfifo_put(&test, value);
>> + mutex_unlock(&write_lock);
>> +out:
>> + return err < 0 ? err : count;
>> }
>>
>> static ssize_t fifo_read(struct file *file, char __user *buf,
>> size_t count, loff_t *ppos)
>> {
>> - int ret;
>> - unsigned int copied;
>> + char buffer[PROC_NUMBUF * FIFO_SIZE];
>> + int value;
>> + size_t len = 0;
>> + ssize_t ret = -1;
>> +
>> + memset(buffer, 0, sizeof(buffer));
>>
>> if (mutex_lock_interruptible(&read_lock))
>> return -ERESTARTSYS;
>>
>> - ret = kfifo_to_user(&test, buf, count, &copied);
>> + while (!kfifo_is_empty(&test)){
>> + ret = kfifo_get(&test, &value);
>> + len = snprintf(buffer, sizeof(buffer), "%s%d\n", buffer, value);
>> + }
>>
>> mutex_unlock(&read_lock);
>> + ret = copy_to_user(buf, buffer, len);
>>
>> - return ret ? ret : copied;
>> + return ret ? ret : len;
>> }
>>
>> static const struct file_operations fifo_fops = {
>
>
>
> .
>