Re: [PATCH 5/5] nvmem: Add support for write-only instances

[Nicholas Johnson]

On Mon, Mar 23, 2020 at 08:05:05PM +0100, Greg KH wrote:
> On Mon, Mar 23, 2020 at 03:00:07PM +0000, Srinivas Kandagatla wrote:
> > From: Nicholas Johnson <nicholas.johnson-opensource@xxxxxxxxxxxxxx>
> > 
> > There is at least one real-world use-case for write-only nvmem
> > instances. Refer to 03cd45d2e219 ("thunderbolt: Prevent crash if
> > non-active NVMem file is read").
> > 
> > Add support for write-only nvmem instances by adding attrs for 0200.
> > 
> > Change nvmem_register() to abort if NULL group is returned from
> > nvmem_sysfs_get_groups().
> > 
> > Return NULL from nvmem_sysfs_get_groups() in invalid cases.
> > 
> > Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@xxxxxxxxxxxxxx>
> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> > ---
> >  drivers/nvmem/core.c        | 10 +++++--
> >  drivers/nvmem/nvmem-sysfs.c | 56 +++++++++++++++++++++++++++++++------
> >  2 files changed, 56 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > index 77d890d3623d..ddc7be5149d5 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -381,6 +381,14 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> >  	nvmem->type = config->type;
> >  	nvmem->reg_read = config->reg_read;
> >  	nvmem->reg_write = config->reg_write;
> > +	nvmem->dev.groups = nvmem_sysfs_get_groups(nvmem, config);
> > +	if (!nvmem->dev.groups) {
> > +		ida_simple_remove(&nvmem_ida, nvmem->id);
> > +		gpiod_put(nvmem->wp_gpio);
> > +		kfree(nvmem);
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> >  	if (!config->no_of_node)
> >  		nvmem->dev.of_node = config->dev->of_node;
> >  
> > @@ -395,8 +403,6 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> >  	nvmem->read_only = device_property_present(config->dev, "read-only") ||
> >  			   config->read_only || !nvmem->reg_write;
> >  
> > -	nvmem->dev.groups = nvmem_sysfs_get_groups(nvmem, config);
> > -
> >  	device_initialize(&nvmem->dev);
> >  
> >  	dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
> > diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c
> > index 8759c4470012..9728da948988 100644
> > --- a/drivers/nvmem/nvmem-sysfs.c
> > +++ b/drivers/nvmem/nvmem-sysfs.c
> > @@ -202,16 +202,49 @@ static const struct attribute_group *nvmem_ro_root_dev_groups[] = {
> >  	NULL,
> >  };
> >  
> > +/* write only permission, root only */
> > +static struct bin_attribute bin_attr_wo_root_nvmem = {
> > +	.attr	= {
> > +		.name	= "nvmem",
> > +		.mode	= 0200,
> > +	},
> > +	.write	= bin_attr_nvmem_write,
> > +};
> 
> You are adding a new sysfs file without a Documentation/ABI/ new entry
> as well?
It is the same existing sysfs file, but adding the ability for that 
existing sysfs file to have write-only (0200) permissions if the driver 
requires it. The perms / groups for the nvmem that is getting registered 
in nvmem_register() in drivers/nvmem/core.c are chosen by 
nvmem_sysfs_get_groups() in drivers/nvmem/nvmem-sysfs.c, and this new 
0200 will get selected if reg_read is NULL and reg_write is provided 
(and nvmem->read_only override is not set).

> 
> 
> > +
> > +static struct bin_attribute *nvmem_bin_wo_root_attributes[] = {
> > +	&bin_attr_wo_root_nvmem,
> > +	NULL,
> > +};
> > +
> > +static const struct attribute_group nvmem_bin_wo_root_group = {
> > +	.bin_attrs	= nvmem_bin_wo_root_attributes,
> > +	.attrs		= nvmem_attrs,
> > +};
> > +
> > +static const struct attribute_group *nvmem_wo_root_dev_groups[] = {
> > +	&nvmem_bin_wo_root_group,
> > +	NULL,
> > +};
> > +
> >  const struct attribute_group **nvmem_sysfs_get_groups(
> >  					struct nvmem_device *nvmem,
> >  					const struct nvmem_config *config)
> >  {
> > -	if (config->root_only)
> > -		return nvmem->read_only ?
> > -			nvmem_ro_root_dev_groups :
> > -			nvmem_rw_root_dev_groups;
> > +	/* Read-only */
> > +	if (nvmem->reg_read && (!nvmem->reg_write || nvmem->read_only))
> > +		return config->root_only ?
> > +			nvmem_ro_root_dev_groups : nvmem_ro_dev_groups;
> > +
> > +	/* Read-write */
> > +	if (nvmem->reg_read && nvmem->reg_write && !nvmem->read_only)
> > +		return config->root_only ?
> > +			nvmem_rw_root_dev_groups : nvmem_rw_dev_groups;
> > +
> > +	/* Write-only, do not honour request for global writable entry */
> > +	if (!nvmem->reg_read && nvmem->reg_write && !nvmem->read_only)
> > +		return config->root_only ? nvmem_wo_root_dev_groups : NULL;
> 
> 
> What is this supposed to be doing, I am totally lost.
This nvmem_sysfs_get_groups() is called by nvmem_register() when the 
nvmem is registered by a driver. This returns the filesystem perms for 
the nvmem based on the inputs (or NULL to abort). There are four things 
we care about in the function arguments:

1. nvmem->reg_read which is a function pointer used when reading from 
the nvmem

2. nvmem->reg_write which is a function pointer used when writing to the 
nvmem

3. nvmem->read_only which overrides if nvmem is read only

4. config->root_only which indicates if world perms are used

We use these to decide which perms to return. We can determine whether 
read-only or write-only or read-write by whether the reg_read and 
reg_write are provided, using nvmem->read_only as an override (sometimes 
from dtb, both reg_read and reg_write are provided but it can override 
to read only with this flag).

Once that is decided, we use config->root_only to determine if 
world-accessible. We return the appropriate group, or NULL if 
world-writable requested.

Before this patch, we could not return NULL (always had to return a 
group, no matter what). This had to be fixed, hence the changes to the 
caller (nvmem_register() function in drivers/nvmem/core.c) to check for 
NULL group and abort. Before this patch, any (unsupported) request for 
write-only was interpreted as read-write, instead of returning an error 
and failing cleanly. As of this patch, the above 0200 which I am 
introducing is returned.

Please let me know if I have failed to clarify this for you and I will 
try again. :)
> 
> greg k-h

Kind regards,
Nicholas Johnson