debugfs question ...

['Greg KH']

On Wed, May 01, 2019 at 03:18:51PM +0100, rdq at 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