Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2 1/3] nvmem: Add support for write-only instances
To:     Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc:     Mika Westerberg <mika.westerberg@linux.intel.com>
References: <PSXP216MB0438930B1FC30EF79F15FD1780E70@PSXP216MB0438.KORP216.PROD.OUTLOOK.COM>
From:   Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Message-ID: <b934ddde-702a-1731-034d-1682a9df23e2@linaro.org>
Date:   Thu, 5 Mar 2020 17:03:11 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <PSXP216MB0438930B1FC30EF79F15FD1780E70@PSXP216MB0438.KORP216.PROD.OUTLOOK.COM>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 02/03/2020 15:42, Nicholas Johnson wrote:
> 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@outlook.com.au>
> ---
>   drivers/nvmem/core.c        |  2 ++
>   drivers/nvmem/nvmem-sysfs.c | 53 ++++++++++++++++++++++++++++++++-----
>   2 files changed, 48 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index ef326f243..27bd4c4e3 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -388,6 +388,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>   			   config->read_only || !nvmem->reg_write;
>   
>   	nvmem->dev.groups = nvmem_sysfs_get_groups(nvmem, config);
> +	if (!nvmem->dev.groups)
> +		return NULL;
Returning here will be leaking in this function.

>   
>   	device_initialize(&nvmem->dev);
>   
> diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c
> index 9e0c429cd..00d3259ea 100644
> --- a/drivers/nvmem/nvmem-sysfs.c
> +++ b/drivers/nvmem/nvmem-sysfs.c
> @@ -196,16 +196,50 @@ 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 = {

TBH, you would not need this patch once 2/3 patch is applied.
Unless there is a strong reason for you to have write only file.

If for any reasons you would want to add Write only file then it should 
be added for both with root and user privileges.

>   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;
> -
> -	return nvmem->read_only ? nvmem_ro_dev_groups : nvmem_rw_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)

read_only flag will override this assumption!

> +		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)
> +		return config->root_only ? nvmem_wo_root_dev_groups : NULL;
> +
> +	/* Neither reg_read nor reg_write are provided, abort */
This should not be the case anymore after this check in place

https://git.kernel.org/pub/scm/linux/kernel/git/srini/nvmem.git/commit/?h=for-next&id=f8f782f63bace8b08362e466747e648ca57b6c06

thanks,
srini

> +	return NULL;
>   }
>   
>   /*
> @@ -224,11 +258,16 @@ int nvmem_sysfs_setup_compat(struct nvmem_device *nvmem,
>   	if (!config->base_dev)
>   		return -EINVAL;
>   
> -	if (nvmem->read_only) {
> +	if (nvmem->reg_read && (!nvmem->reg_write || nvmem->read_only)) {
>   		if (config->root_only)
>   			nvmem->eeprom = bin_attr_ro_root_nvmem;
>   		else
>   			nvmem->eeprom = bin_attr_ro_nvmem;
> +	} else if (!nvmem->reg_read && nvmem->reg_write) {
> +		if (config->root_only)
> +			nvmem->eeprom = bin_attr_wo_root_nvmem;
> +		else
> +			return -EPERM;
>   	} else {
>   		if (config->root_only)
>   			nvmem->eeprom = bin_attr_rw_root_nvmem;
>