debugfs question ...

debugfs question ...

[rdq]

Greetings,

My I2C sensor driver has a debugfs entry for development purposes.
Everything works fine with the exception of the read operation. When 'cat'
is used, the read operation is called repeatedly and indefinitely. If the
read() is changed to return 0 then, as expected, nothing is displayed. 

The pattern for the implementation is (AFAICT) right out of  the book (shown
below). 

What am I missing? Any thoughts much appreciated.

TAIA.

RDQ

static ssize_t sc031gs_reg_read_file(struct file *file, char __user
*user_buf,
				   size_t count, loff_t *ppos)
{
	char *buf = 0;
	ssize_t total = 0;
	struct sc031gs_dev *sensor = file->private_data;
	if (!sensor)
		return -EINVAL;
	if (*ppos < 0 || !count)
		return -EINVAL;
	buf = kmalloc(count, GFP_KERNEL);
	if (!buf)
		return -ENOMEM;
	total = snprintf(buf,count,"Hello world\n");
	if (total >= 0) {
		if (copy_to_user(user_buf, buf, total)) {
			kfree(buf);
			return -EFAULT;
		}
		*ppos += total;
	}
	kfree(buf);
	return total;
}


_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: debugfs question ...

[Greg KH]

On Tue, Apr 30, 2019 at 12:45:08PM +0100, rdq@metamail.co wrote:
> Greetings,
> 
> My I2C sensor driver has a debugfs entry for development purposes.
> Everything works fine with the exception of the read operation. When 'cat'
> is used, the read operation is called repeatedly and indefinitely. If the
> read() is changed to return 0 then, as expected, nothing is displayed. 
> 
> The pattern for the implementation is (AFAICT) right out of  the book (shown
> below). 
> 
> What am I missing? Any thoughts much appreciated.
> 
> TAIA.
> 
> RDQ
> 
> static ssize_t sc031gs_reg_read_file(struct file *file, char __user
> *user_buf,
> 				   size_t count, loff_t *ppos)
> {
> 	char *buf = 0;
> 	ssize_t total = 0;
> 	struct sc031gs_dev *sensor = file->private_data;
> 	if (!sensor)
> 		return -EINVAL;
> 	if (*ppos < 0 || !count)
> 		return -EINVAL;
> 	buf = kmalloc(count, GFP_KERNEL);
> 	if (!buf)
> 		return -ENOMEM;
> 	total = snprintf(buf,count,"Hello world\n");
> 	if (total >= 0) {
> 		if (copy_to_user(user_buf, buf, total)) {
> 			kfree(buf);
> 			return -EFAULT;
> 		}
> 		*ppos += total;
> 	}
> 	kfree(buf);
> 	return total;
> }

You are returning a "short" read, and then disallowing ppos to be set to
a non-zero value?  That's a recipie for disaster :(

Also, you allow userspace to allocate as much memory as it asks for?
Not good :(

Why not just use the simple_read_from_buffer() call?  That handles all
of the "housekeeping" for you, and your function can be _much_ simpler.

good luck!

greg k-h

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: debugfs question ...

[Martin Christian]

[-- Attachment #1.1.1: Type: text/plain, Size: 1897 bytes --]

Hi,

my guess is your are not returning EOF (= 0). User space expects a
return value of 0 (EOF) to terminate reading. But your code will always
return sizeof("Hello world\n"). A fix would be to not only increment
ppos, but also check if it's already behind your data.

Regards,

Martin


Am 30.04.19 um 13:45 schrieb rdq@metamail.co:
> Greetings,
> 
> My I2C sensor driver has a debugfs entry for development purposes.
> Everything works fine with the exception of the read operation. When 'cat'
> is used, the read operation is called repeatedly and indefinitely. If the
> read() is changed to return 0 then, as expected, nothing is displayed. 
> 
> The pattern for the implementation is (AFAICT) right out of  the book (shown
> below). 
> 
> What am I missing? Any thoughts much appreciated.
> 
> TAIA.
> 
> RDQ
> 
> static ssize_t sc031gs_reg_read_file(struct file *file, char __user
> *user_buf,
> 				   size_t count, loff_t *ppos)
> {
> 	char *buf = 0;
> 	ssize_t total = 0;
> 	struct sc031gs_dev *sensor = file->private_data;
> 	if (!sensor)
> 		return -EINVAL;
> 	if (*ppos < 0 || !count)
> 		return -EINVAL;
> 	buf = kmalloc(count, GFP_KERNEL);
> 	if (!buf)
> 		return -ENOMEM;
> 	total = snprintf(buf,count,"Hello world\n");
> 	if (total >= 0) {
> 		if (copy_to_user(user_buf, buf, total)) {
> 			kfree(buf);
> 			return -EFAULT;
> 		}
> 		*ppos += total;
> 	}
> 	kfree(buf);
> 	return total;
> }
> 
> 
> _______________________________________________
> Kernelnewbies mailing list
> Kernelnewbies@kernelnewbies.org
> https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
> 

-- 
Dipl.-Inf. Martin Christian
Senior Berater Entwicklung Hardware
secunet Security Networks AG

Tel.: +49 201 5454-3612, Fax +49 201 5454-1323
E-Mail: martin.christian@secunet.com
Ammonstraße 74, 01067 Dresden
www.secunet.com


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

RE: debugfs question ...

[rdq]

> my guess is your are not returning EOF (= 0). User space expects a return
> value of 0 (EOF) to terminate reading. But your code will always return
> sizeof("Hello world\n"). A fix would be to not only increment ppos, but also
> check if it's already behind your data.
> 
> Regards,
> 
> Martin

Thank you, Martin, help much appreciated. It's a bit of a mystery but neither works for me. More detail in my reply to Greg KH.



_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: debugfs question ...

['Greg KH']

On Wed, May 01, 2019 at 03:18:51PM +0100, rdq@metamail.co wrote:
> Hello and thanks,
> 
> > >
> > > The pattern for the implementation is (AFAICT) right out of  the book
> > >
> > 
> > You are returning a "short" read, and then disallowing ppos to be set to a
> > non-zero value?  That's a recipie for disaster :(
> > 
> > Also, you allow userspace to allocate as much memory as it asks for?
> > Not good :(
> 
> Well I started here:
> https://github.com/torvalds/linux/blob/v4.14/sound/soc/soc-core.c#L251.
> 
> > Why not just use the simple_read_from_buffer() call?  That handles all of
> the
> > "housekeeping" for you, and your function can be _much_ simpler.
> 
> Possibly but it does not explain why returning 0 means cat displays nothing

You returned nothing, so what is cat supposed to do?  It worked
properly.

> and returning the string length causes it to loop indefinitely. 

You have to give userspace what it asks for, don't ignore the ppos
variable.

Have you tried writing a userspace program that does a correct read()
call on a file or device node like this?  You have to keep reading until
you get all of the data you asked for, or the kernel returns
end-of-file, or an error happens.  You don't just do a single read and
hope for the best :)

> My example now reduced to:
> 
> 	int ret;
> 	char buf[64];
> 	int n = snprintf(buf,64,"Hello World\n");
> 	if (n < 0)
> 		return -EINVAL;
> 	// zero on success
> 	ret = copy_to_user(user_buf,buf,n);
> 	*ppos += n;	
> 	return ret;
> 
> Is this the canonical form?

Not at all, that's a recipe for disaster.

> Using a 4.14 kernel, cat displays nothing. Or has something changed?
> If so mighty odd as there are ancient example as well as the new:
> 
> https://sites.google.com/site/linuxkernel88/sample-code/writing-a-character-driver in char_device_read()

A char device is MUCH different from a file interface that you are using
here.  It kind of looks the same, but it has some subtle ways in which
things can go sideways very easily.

Also, that code is really really wrong, if that char_device_read()
function in the above link was ever submitted to me, I would have a
field day with it.  Nothing like bad random internet examples to steer
people in the wrong direction :(

> https://github.com/torvalds/linux/blob/master/drivers/base/regmap/regmap-debugfs.c#L254

That one is not "quite" like your example here either.

You have the option to handle all of the fun corner cases and error
values by writing it yourself, or just use the helper function I pointed
you at which does all of what you need to do, correctly.  To do
something in the middle is to guarantee that something will not work
properly with some sort of userspace program, or be totally insecure.

thanks,

greg k-h

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies