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

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

[Wang Long]

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

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

[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 <long.wanglong@huawei.com>
---
 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

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

[Stefani Seibold]

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 <long.wanglong@huawei.com>
> ---
>  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 = {

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

[long.wanglong]

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 <long.wanglong@huawei.com>
>> ---
>>  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 = {
> 
> 
> 
> .
>