Re: [PATCH 2/2] nvme: identify-namespace without CAP_SYS_ADMIN

[Kanchan Joshi <joshi.k@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 2309 bytes --]

On Mon, Oct 31, 2022 at 06:55:56AM +0000, Chaitanya Kulkarni wrote:
>On 10/20/22 00:02, Kanchan Joshi wrote:
>> Allow all identify-namespace variants (CNS 00h, 05h and 08h) without
>> requiring CAP_SYS_ADMIN. The information (retrieved using id-ns) is
>> needed to form IO commands for passthrough interface.
>>
>> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
>> ---
>>   drivers/nvme/host/ioctl.c | 14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
>> index 9c581b1a8956..9273db147872 100644
>> --- a/drivers/nvme/host/ioctl.c
>> +++ b/drivers/nvme/host/ioctl.c
>> @@ -15,9 +15,19 @@ bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c, fmode_t mode)
>>   	if (capable(CAP_SYS_ADMIN))
>>   		return true;
>>
>> -	/* admin commands are not allowed */
>> -	if (!ns)
>> +	/* policy for admin commands */
>
>above comment is not needed as it is clear from the opcode below
>you are dealing with admin commands only that too specific cns
>values ..
>
>> +	if (!ns) {
>> +		if (opcode == nvme_admin_identify) {
>> +			switch (c->identify.cns) {
>> +			case NVME_ID_CNS_NS:
>> +			case NVME_ID_CNS_CS_NS:
>> +			case NVME_ID_CNS_NS_CS_INDEP:
>> +				return true;
>> +			}
>> +		}
>> +		/* other admin commands are not allowed */
>
>same here..

All right, will kill these. 
>
>>   		return false;
>
>if and swicth and two returns are looking confusing, I'd use
>nested switch case default here..

Do you think that'll give better looking code?
I did not write that because it did not seem good fit for the
situtation. It involved aligning more curly braces: 

-       /* admin commands are not allowed */
-       if (!ns)
+       if (!ns) {
+               switch (opcode) {
+               case nvme_admin_identify: {
+                       switch (c->identify.cns) {
+                       case NVME_ID_CNS_NS:
+                       case NVME_ID_CNS_CS_NS:
+                       case NVME_ID_CNS_NS_CS_INDEP:
+                               return true;
+                       }
+               }
+               }
                return false;
+       }

Above is without default. And with two defaults, it just gets more
wordy.
And future growth in above admin opcodes is not expected too.

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