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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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;
> }
>
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.
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
[email protected] +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
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 <[email protected]>
> ---
> drivers/nvme/target/fcloop.c | 29 +++++++++++++++++++++++------
> 1 file changed, 23 insertions(+), 6 deletions(-)
>
>
Reviewed-by: James Smart <[email protected]>
Looks good. Sorry for the delay.
-- james
Reviewed-by: Sagi Grimberg <[email protected]>
Hi Christoph,
Would you mind apply this one with the Reviewed-by from James and Sagi?
https://lore.kernel.org/linux-nvme/[email protected]/
https://lore.kernel.org/linux-nvme/[email protected]/
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 <[email protected]>
>> ---
>> drivers/nvme/target/fcloop.c | 29 +++++++++++++++++++++++------
>> 1 file changed, 23 insertions(+), 6 deletions(-)
>>
>>
>
> Reviewed-by: James Smart <[email protected]>
>
> Looks good. Sorry for the delay.
>
> -- james
>
>