Re: [PATCH v10 4/8] PCI/sysfs: Allow userspace to query and set device reset mechanism

["Krzysztof Wilczyński" <kw@linux.com>]

Hi Bjorn,

[...]
> > +	if (count >= (PAGE_SIZE - 1))
> > +		return -EINVAL;
> 
> I'm not the sysfs expert, but surely the sysfs infrastructure already
> guarantees this?

We don't need to store any value, since we are processing the input from
the userspace, thus ensuring that we have room for the newline is not
needed, especially since the show() function dynamically builds the
content to show, so indeed this check can be dropped.

To add, there aren't any guarantees other from sysfs than we get a up to
a PAGE_SIZE worth of data in the buffer.

[...]
> > +	options = kstrndup(buf, count, GFP_KERNEL);
> 
> I assume the kstrndup() is because strsep() writes into the buffer?

Yes, Amey added kstrndup() in v6 following my recommendation as per:

  https://lore.kernel.org/linux-pci/20210606125800.GA76573@rocinante.localdomain/

This was to avoid removing the const quantifier through a type cast
given that the signature of the function denotes that the buffer is
a pointer to immutable string, as per:

  https://elixir.bootlin.com/linux/v5.14-rc3/source/include/linux/device/driver.h#L137

Some other sysfs users do employ the cast when using strtok() and I am
not so such it's the right way to do it, as per:

  drivers/s390/net/qeth_l3_sys.c
  232:	tmp = strsep((char **)&buf, "\n");
  
  drivers/media/rc/rc-main.c
  1167:	while ((tmp = strsep((char **)&buf, " \n")) != NULL) {

> Aren't we allowed to write into the buffer we get from sysfs?  Does
> the user ever see the buffer contents again?  I would think sysfs
> would have already done a copy_from_user() or whatever.

I might be wrong about this, but I suppose this might be to stop people
from accidentally freeing the buffer as kernfs_fop_write_iter() would do
it after all the internal housekeeping is done, provided that someone
pays attention to compile time warnings.

	Krzysztof