[PATCH 1/1] nvme-fcloop: verify wwnn and wwpn format

[PATCH 1/1] nvme-fcloop: verify wwnn and wwpn format

[Dongli Zhang]

The nvme host and target verify the wwnn and wwpn format via
nvme_fc_parse_traddr(). For instance, it is required that the length of
wwnn to be either 21 ("nn-0x") or 19 (nn-).

Add this verification to nvme-fcloop so that the input should always be in
hex and the length of input should always be 18.

Otherwise, the user may use e.g. 0x2 to create fcloop local port, while
0x0000000000000002 is required for nvme host and target. This makes the
requirement of format confusing.

Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
 drivers/nvme/target/fcloop.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
index f69ce66e2d44..14124e6d4bf2 100644
--- a/drivers/nvme/target/fcloop.c
+++ b/drivers/nvme/target/fcloop.c
@@ -43,6 +43,17 @@ static const match_table_t opt_tokens = {
 	{ NVMF_OPT_ERR,		NULL		}
 };
 
+static int fcloop_verify_addr(substring_t *s)
+{
+	size_t blen = s->to - s->from + 1;
+
+	if (strnlen(s->from, blen) != NVME_FC_TRADDR_HEXNAMELEN + 2 ||
+	    strncmp(s->from, "0x", 2))
+		return -EINVAL;
+
+	return 0;
+}
+
 static int
 fcloop_parse_options(struct fcloop_ctrl_options *opts,
 		const char *buf)
@@ -64,14 +75,16 @@ fcloop_parse_options(struct fcloop_ctrl_options *opts,
 		opts->mask |= token;
 		switch (token) {
 		case NVMF_OPT_WWNN:
-			if (match_u64(args, &token64)) {
+			if (fcloop_verify_addr(args) ||
+			    match_u64(args, &token64)) {
 				ret = -EINVAL;
 				goto out_free_options;
 			}
 			opts->wwnn = token64;
 			break;
 		case NVMF_OPT_WWPN:
-			if (match_u64(args, &token64)) {
+			if (fcloop_verify_addr(args) ||
+			    match_u64(args, &token64)) {
 				ret = -EINVAL;
 				goto out_free_options;
 			}
@@ -92,14 +105,16 @@ fcloop_parse_options(struct fcloop_ctrl_options *opts,
 			opts->fcaddr = token;
 			break;
 		case NVMF_OPT_LPWWNN:
-			if (match_u64(args, &token64)) {
+			if (fcloop_verify_addr(args) ||
+			    match_u64(args, &token64)) {
 				ret = -EINVAL;
 				goto out_free_options;
 			}
 			opts->lpwwnn = token64;
 			break;
 		case NVMF_OPT_LPWWPN:
-			if (match_u64(args, &token64)) {
+			if (fcloop_verify_addr(args) ||
+			    match_u64(args, &token64)) {
 				ret = -EINVAL;
 				goto out_free_options;
 			}
@@ -141,14 +156,16 @@ fcloop_parse_nm_options(struct device *dev, u64 *nname, u64 *pname,
 		token = match_token(p, opt_tokens, args);
 		switch (token) {
 		case NVMF_OPT_WWNN:
-			if (match_u64(args, &token64)) {
+			if (fcloop_verify_addr(args) ||
+			    match_u64(args, &token64)) {
 				ret = -EINVAL;
 				goto out_free_options;
 			}
 			*nname = token64;
 			break;
 		case NVMF_OPT_WWPN:
-			if (match_u64(args, &token64)) {
+			if (fcloop_verify_addr(args) ||
+			    match_u64(args, &token64)) {
 				ret = -EINVAL;
 				goto out_free_options;
 			}
-- 
2.17.1

Re: [PATCH 1/1] nvme-fcloop: verify wwnn and wwpn format

[Dongli Zhang]

May I get feedback for this?

For the first time I use fcloop, I set:

# echo "wwnn=0x3,wwpn=0x1" > /sys/class/fcloop/ctl/add_target_port

However, I would not be able to move forward if I use "0x3" or "0x1" for nvme-fc
target or host further. Instead, the address and port should be
0x0000000000000003 and 0x0000000000000001.

This patch would sync the requirements of input format for nvme-fc and
nvme-fcloop, unless this would break existing test suite (e.g., blktest).

Thank you very much!

Dongli Zhang

On 5/25/20 9:21 PM, Dongli Zhang wrote:
> The nvme host and target verify the wwnn and wwpn format via
> nvme_fc_parse_traddr(). For instance, it is required that the length of
> wwnn to be either 21 ("nn-0x") or 19 (nn-).
> 
> Add this verification to nvme-fcloop so that the input should always be in
> hex and the length of input should always be 18.
> 
> Otherwise, the user may use e.g. 0x2 to create fcloop local port, while
> 0x0000000000000002 is required for nvme host and target. This makes the
> requirement of format confusing.
> 
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
> ---
>  drivers/nvme/target/fcloop.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> index f69ce66e2d44..14124e6d4bf2 100644
> --- a/drivers/nvme/target/fcloop.c
> +++ b/drivers/nvme/target/fcloop.c
> @@ -43,6 +43,17 @@ static const match_table_t opt_tokens = {
>  	{ NVMF_OPT_ERR,		NULL		}
>  };
>  
> +static int fcloop_verify_addr(substring_t *s)
> +{
> +	size_t blen = s->to - s->from + 1;
> +
> +	if (strnlen(s->from, blen) != NVME_FC_TRADDR_HEXNAMELEN + 2 ||
> +	    strncmp(s->from, "0x", 2))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static int
>  fcloop_parse_options(struct fcloop_ctrl_options *opts,
>  		const char *buf)
> @@ -64,14 +75,16 @@ fcloop_parse_options(struct fcloop_ctrl_options *opts,
>  		opts->mask |= token;
>  		switch (token) {
>  		case NVMF_OPT_WWNN:
> -			if (match_u64(args, &token64)) {
> +			if (fcloop_verify_addr(args) ||
> +			    match_u64(args, &token64)) {
>  				ret = -EINVAL;
>  				goto out_free_options;
>  			}
>  			opts->wwnn = token64;
>  			break;
>  		case NVMF_OPT_WWPN:
> -			if (match_u64(args, &token64)) {
> +			if (fcloop_verify_addr(args) ||
> +			    match_u64(args, &token64)) {
>  				ret = -EINVAL;
>  				goto out_free_options;
>  			}
> @@ -92,14 +105,16 @@ fcloop_parse_options(struct fcloop_ctrl_options *opts,
>  			opts->fcaddr = token;
>  			break;
>  		case NVMF_OPT_LPWWNN:
> -			if (match_u64(args, &token64)) {
> +			if (fcloop_verify_addr(args) ||
> +			    match_u64(args, &token64)) {
>  				ret = -EINVAL;
>  				goto out_free_options;
>  			}
>  			opts->lpwwnn = token64;
>  			break;
>  		case NVMF_OPT_LPWWPN:
> -			if (match_u64(args, &token64)) {
> +			if (fcloop_verify_addr(args) ||
> +			    match_u64(args, &token64)) {
>  				ret = -EINVAL;
>  				goto out_free_options;
>  			}
> @@ -141,14 +156,16 @@ fcloop_parse_nm_options(struct device *dev, u64 *nname, u64 *pname,
>  		token = match_token(p, opt_tokens, args);
>  		switch (token) {
>  		case NVMF_OPT_WWNN:
> -			if (match_u64(args, &token64)) {
> +			if (fcloop_verify_addr(args) ||
> +			    match_u64(args, &token64)) {
>  				ret = -EINVAL;
>  				goto out_free_options;
>  			}
>  			*nname = token64;
>  			break;
>  		case NVMF_OPT_WWPN:
> -			if (match_u64(args, &token64)) {
> +			if (fcloop_verify_addr(args) ||
> +			    match_u64(args, &token64)) {
>  				ret = -EINVAL;
>  				goto out_free_options;
>  			}
> 

Re: [PATCH 1/1] nvme-fcloop: verify wwnn and wwpn format

[Chaitanya Kulkarni]

On 6/3/20 11:46 PM, Dongli Zhang wrote:
> May I get feedback for this?
> 
> For the first time I use fcloop, I set:
> 
> # echo "wwnn=0x3,wwpn=0x1" > /sys/class/fcloop/ctl/add_target_port
> 
> However, I would not be able to move forward if I use "0x3" or "0x1" for nvme-fc
> target or host further. Instead, the address and port should be
> 0x0000000000000003 and 0x0000000000000001.
> 
> This patch would sync the requirements of input format for nvme-fc and
> nvme-fcloop, unless this would break existing test suite (e.g., blktest).
If I remember correctly I don't think we have fc-loop testcases (correct 
me if I'm wrong).

Not an fc expert, but having uniform format for the input make sense to 
me (unless there is an explicit reason). I'll let James have a final say.

Re: [PATCH 1/1] nvme-fcloop: verify wwnn and wwpn format

[Hannes Reinecke]

On 6/4/20 8:54 AM, Chaitanya Kulkarni wrote:
> On 6/3/20 11:46 PM, Dongli Zhang wrote:
>> May I get feedback for this?
>>
>> For the first time I use fcloop, I set:
>>
>> # echo "wwnn=0x3,wwpn=0x1" > /sys/class/fcloop/ctl/add_target_port
>>
>> However, I would not be able to move forward if I use "0x3" or "0x1" for nvme-fc
>> target or host further. Instead, the address and port should be
>> 0x0000000000000003 and 0x0000000000000001.
>>
>> This patch would sync the requirements of input format for nvme-fc and
>> nvme-fcloop, unless this would break existing test suite (e.g., blktest).
> If I remember correctly I don't think we have fc-loop testcases (correct
> me if I'm wrong).
> 
Well, I sent some testcases a while back (cf 'fcloop and ANA fixes').
Should I resend them?

> Not an fc expert, but having uniform format for the input make sense to
> me (unless there is an explicit reason). I'll let James have a final say.
> 

I would stick to use the full 64bit number for both wwpn and wwnn; one 
gets into too many arguments otherwise (big-endian? little-endian?).
And one could argue that '0x0000000000000001' is invalid anyway as per 
FC-FS3 a '0' in word 0 byte 0 means 'Name not present' :-)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                               +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 1/1] nvme-fcloop: verify wwnn and wwpn format

[James Smart]

On 5/25/2020 9:21 PM, Dongli Zhang wrote:
> The nvme host and target verify the wwnn and wwpn format via
> nvme_fc_parse_traddr(). For instance, it is required that the length of
> wwnn to be either 21 ("nn-0x") or 19 (nn-).
>
> Add this verification to nvme-fcloop so that the input should always be in
> hex and the length of input should always be 18.
>
> Otherwise, the user may use e.g. 0x2 to create fcloop local port, while
> 0x0000000000000002 is required for nvme host and target. This makes the
> requirement of format confusing.
>
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
> ---
>   drivers/nvme/target/fcloop.c | 29 +++++++++++++++++++++++------
>   1 file changed, 23 insertions(+), 6 deletions(-)
>
>

Reviewed-by: James Smart <james.smart@broadcom.com>

Looks good. Sorry for the delay.

-- james

Re: [PATCH 1/1] nvme-fcloop: verify wwnn and wwpn format

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 1/1] nvme-fcloop: verify wwnn and wwpn format

[Dongli Zhang]

Hi Christoph,

Would you mind apply this one with the Reviewed-by from James and Sagi?

https://lore.kernel.org/linux-nvme/60df6752-3512-f7a9-b0df-1096b93b8eda@broadcom.com/

https://lore.kernel.org/linux-nvme/c4ec2d9e-b08c-19b2-16a5-93520ca13c2e@grimberg.me/

Thank you very much!

Dongli Zhang

On 6/4/20 8:20 AM, James Smart wrote:
> On 5/25/2020 9:21 PM, Dongli Zhang wrote:
>> The nvme host and target verify the wwnn and wwpn format via
>> nvme_fc_parse_traddr(). For instance, it is required that the length of
>> wwnn to be either 21 ("nn-0x") or 19 (nn-).
>>
>> Add this verification to nvme-fcloop so that the input should always be in
>> hex and the length of input should always be 18.
>>
>> Otherwise, the user may use e.g. 0x2 to create fcloop local port, while
>> 0x0000000000000002 is required for nvme host and target. This makes the
>> requirement of format confusing.
>>
>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
>> ---
>>   drivers/nvme/target/fcloop.c | 29 +++++++++++++++++++++++------
>>   1 file changed, 23 insertions(+), 6 deletions(-)
>>
>>
> 
> Reviewed-by: James Smart <james.smart@broadcom.com>
> 
> Looks good. Sorry for the delay.
> 
> -- james
> 
> 

Re: [PATCH 1/1] nvme-fcloop: verify wwnn and wwpn format

[Christoph Hellwig]

Applied to nvme-5.9.