[PATCH v2] nvme: consider also host_iface when checking ip options

[PATCH v2] nvme: consider also host_iface when checking ip options

[Daniel Wagner]

It's perfectly fine to use the same traddr and trsvcid more than once
as long we use different a host_iface. This is used in setups where
the host has more than one interface but the target exposes only one
IP.

To keep the match logic simple don't allow mixed setups. The user
needs to use host_iface option consistently for all controllers in
such setups.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
---


leap154:~ # nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420 

leap154:~ # nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420 
Failed to write to /dev/nvme-fabrics: Operation already in progress
no controller found: failed to write to nvme-fabrics device

leap154:~ # nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420 -f eth1
Failed to write to /dev/nvme-fabrics: Operation already in progress
no controller found: failed to write to nvme-fabrics device

leap154:~ # nvme disconnect -n nvmet-test
NQN:nvmet-test disconnected 1 controller(s)

leap154:~ # nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420 -f eth0

leap154:~ # nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420 -f eth1

 drivers/nvme/host/fabrics.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 604ab0e5a2ad..dc12853d04c9 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -944,7 +944,10 @@ bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
 {
 	if (!nvmf_ctlr_matches_baseopts(ctrl, opts) ||
 	    strcmp(opts->traddr, ctrl->opts->traddr) ||
-	    strcmp(opts->trsvcid, ctrl->opts->trsvcid))
+	    strcmp(opts->trsvcid, ctrl->opts->trsvcid) ||
+	    (((opts->mask & NVMF_OPT_HOST_IFACE) &&
+	      (ctrl->opts->mask & NVMF_OPT_HOST_IFACE)) &&
+	       strcmp(opts->host_iface, ctrl->opts->host_iface)))
 		return false;
 
 	/*
-- 
2.37.1

Re: [PATCH v2] nvme: consider also host_iface when checking ip options

[Chao Leng]

On 2022/7/22 17:24, Daniel Wagner wrote:
> It's perfectly fine to use the same traddr and trsvcid more than once
> as long we use different a host_iface. This is used in setups where
> the host has more than one interface but the target exposes only one
> IP.
> 
> To keep the match logic simple don't allow mixed setups. The user
> needs to use host_iface option consistently for all controllers in
> such setups.
> 
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
> 
> 
> leap154:~ # nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420
> 
> leap154:~ # nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420
> Failed to write to /dev/nvme-fabrics: Operation already in progress
> no controller found: failed to write to nvme-fabrics device
> 
> leap154:~ # nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420 -f eth1
> Failed to write to /dev/nvme-fabrics: Operation already in progress
> no controller found: failed to write to nvme-fabrics device
> 
> leap154:~ # nvme disconnect -n nvmet-test
> NQN:nvmet-test disconnected 1 controller(s)
> 
> leap154:~ # nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420 -f eth0
> 
> leap154:~ # nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420 -f eth1
> 
>   drivers/nvme/host/fabrics.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 604ab0e5a2ad..dc12853d04c9 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -944,7 +944,10 @@ bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
>   {
>   	if (!nvmf_ctlr_matches_baseopts(ctrl, opts) ||
>   	    strcmp(opts->traddr, ctrl->opts->traddr) ||
> -	    strcmp(opts->trsvcid, ctrl->opts->trsvcid))
> +	    strcmp(opts->trsvcid, ctrl->opts->trsvcid) ||
> +	    (((opts->mask & NVMF_OPT_HOST_IFACE) &&
> +	      (ctrl->opts->mask & NVMF_OPT_HOST_IFACE)) &&
> +	       strcmp(opts->host_iface, ctrl->opts->host_iface)))     Maybe we should not check the host_iface, because it is not used in the controller
management code. If some users uses these parameters incorrectly, it may result in
unexpected behavior.

>   		return false;
>   
>   	/*
> 

Re: [PATCH v2] nvme: consider also host_iface when checking ip options

[Daniel Wagner]

On Fri, Jul 22, 2022 at 05:37:42PM +0800, Chao Leng wrote:

> Maybe we should not check the host_iface, because it is not used in the controller
> management code. If some users uses these parameters incorrectly, it may result in
> unexpected behavior.

Currently we only allow 1:1 setups. Checking if we use different network
interfaces allows n:1 setups.

Obviously, if the user has a invalid IP configuration etc things will
break apart but this is true for almost anything.

Re: [PATCH v2] nvme: consider also host_iface when checking ip options

[Chao Leng]

On 2022/7/22 17:46, Daniel Wagner wrote:
> On Fri, Jul 22, 2022 at 05:37:42PM +0800, Chao Leng wrote:
> 
>> Maybe we should not check the host_iface, because it is not used in the controller
>> management code. If some users uses these parameters incorrectly, it may result in
>> unexpected behavior.
> 
> Currently we only allow 1:1 setups. Checking if we use different network
> interfaces allows n:1 setups.
> 
> Obviously, if the user has a invalid IP configuration etc things will
> break apart but this is true for almost anything.
We should add the N:1 related controller management first, and then add
the host_iface cheching. Thus it might be better.
> .
> 

Re: [PATCH v2] nvme: consider also host_iface when checking ip options

[Daniel Wagner]

On Fri, Jul 22, 2022 at 05:55:40PM +0800, Chao Leng wrote:
> We should add the N:1 related controller management first, and then add
> the host_iface cheching. Thus it might be better.

I am not following you here. What do you mean with controller
management?

I've tested this patch against our nvmet backend. This allows additional
connections. E.g. the host has following setup:

 eth0: 192.168.154.50/24
 eth1: 192.168.154.214/24

and the target:

 br0: 192.168.19.54/24

With this change we can do this here:

nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420 -f eth0
nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420 -f eth1

What's wrong with it?

Re: [PATCH v2] nvme: consider also host_iface when checking ip options

[Chao Leng]

On 2022/7/22 18:09, Daniel Wagner wrote:
> On Fri, Jul 22, 2022 at 05:55:40PM +0800, Chao Leng wrote:
>> We should add the N:1 related controller management first, and then add
>> the host_iface cheching. Thus it might be better.
> 
> I am not following you here. What do you mean with controller
> management?
> 
> I've tested this patch against our nvmet backend. This allows additional
> connections. E.g. the host has following setup:
> 
>   eth0: 192.168.154.50/24
>   eth1: 192.168.154.214/24
> 
> and the target:
> 
>   br0: 192.168.19.54/24
> 
> With this change we can do this here:
> 
> nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420 -f eth0
> nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420 -f eth1
> 
> What's wrong with it?
nvme over tcp has implemented host_iface-related processing.
But nvme over rdma(roce) do not.
You can test with nvme over rdma(roce).
> .
> 

Re: [PATCH v2] nvme: consider also host_iface when checking ip options

[Daniel Wagner]

On Mon, Jul 25, 2022 at 08:52:27AM +0800, Chao Leng wrote:
> > What's wrong with it?
> nvme over tcp has implemented host_iface-related processing.
> But nvme over rdma(roce) do not.
> You can test with nvme over rdma(roce).

Ah, now I get your point. Indeed this will only work for the TCP
transport. But the RDMA transport doesn't set the NVMF_OPT_HOST_IFACE to
the allowed_opts so we should filter out this with the additional
NVMF_OPT_HOST_IFACE checks for the RDMA transport.

Re: [PATCH v2] nvme: consider also host_iface when checking ip options

[Chao Leng]

On 2022/7/25 17:25, Daniel Wagner wrote:
> On Mon, Jul 25, 2022 at 08:52:27AM +0800, Chao Leng wrote:
>>> What's wrong with it?
>> nvme over tcp has implemented host_iface-related processing.
>> But nvme over rdma(roce) do not.
>> You can test with nvme over rdma(roce).
> 
> Ah, now I get your point. Indeed this will only work for the TCP
> transport. But the RDMA transport doesn't set the NVMF_OPT_HOST_IFACE to
> the allowed_opts so we should filter out this with the additional
> NVMF_OPT_HOST_IFACE checks for the RDMA transport.
Yes, adding opts->mask filter with NVMF_OPT_HOST_IFACE is correct.
  	if (!nvmf_ctlr_matches_baseopts(ctrl, opts) ||
  	    strcmp(opts->traddr, ctrl->opts->traddr) ||
  	    strcmp(opts->trsvcid, ctrl->opts->trsvcid) ||
-	    strcmp(opts->host_iface, ctrl->opts->host_iface))
+	    (((opts->mask & NVMF_OPT_HOST_IFACE) &&
+	      (ctrl->opts->mask & NVMF_OPT_HOST_IFACE)) &&
+	       strcmp(opts->host_iface, ctrl->opts->host_iface)))
  		return false;
Checking host_iface is similar to host_tradr, so some suggestions:
First, move these codes near checking host_traddr.
Second, the checking logic should be the same as that of host_tradr.
It might be better to add a helper to implement host port attribute
checking. Of course, this can be considered as a separate patch.
> .
> 

Re: [PATCH v2] nvme: consider also host_iface when checking ip options

[Daniel Wagner]

On Tue, Jul 26, 2022 at 10:11:41AM +0800, Chao Leng wrote:
> Checking host_iface is similar to host_tradr, so some suggestions:
> First, move these codes near checking host_traddr.

We have several places where we have the same sequence as above, that is
traddr, trsvcid and then host_iface, e.g. nvme_class_uevent,
nvmf_free_options, nvmf_get_address. So I'd say we should stick to this
and not start introducing a new ordering scheme.

> Second, the checking logic should be the same as that of host_tradr.

What do you mean with this? We compare the host_iface with strcmp. It is
exactly the same thing we do with traddr and trsvcid.

> It might be better to add a helper to implement host port attribute
> checking. Of course, this can be considered as a separate patch.

As far I can tell Christoph is not a big fan of small one-line helpers
especially if there is only one call place. But as you said, that's his
call :)

Re: [PATCH v2] nvme: consider also host_iface when checking ip options

[Chao Leng]

On 2022/7/26 15:18, Daniel Wagner wrote:
> On Tue, Jul 26, 2022 at 10:11:41AM +0800, Chao Leng wrote:
>> Checking host_iface is similar to host_tradr, so some suggestions:
>> First, move these codes near checking host_traddr.
> 
> We have several places where we have the same sequence as above, that is
> traddr, trsvcid and then host_iface, e.g. nvme_class_uevent,
> nvmf_free_options, nvmf_get_address. So I'd say we should stick to this
> and not start introducing a new ordering scheme.
> 
>> Second, the checking logic should be the same as that of host_tradr.
> 
> What do you mean with this? We compare the host_iface with strcmp. It is
> exactly the same thing we do with traddr and trsvcid.
host_iface is different with traddr and trsvcid.
traddr is required opt, trsvcid can be specified or the default.
host_iface is NULL if without specified. It is same with host_tradr.

For example:
nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420 -f eth0
nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420
According to your patch, the second will be treated as a duplicate.
It is similar like this:
nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420 -w 192.168.154.50
nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420
This will be treated as a different new.

host_iface and host_tradr are similar opts related to host outgoing.
host_iface and host_tradr may or may not be specified.
The check logic should be consistent.
> 
>> It might be better to add a helper to implement host port attribute
>> checking. Of course, this can be considered as a separate patch.
> 
> As far I can tell Christoph is not a big fan of small one-line helpers
> especially if there is only one call place. But as you said, that's his
> call :)
> .
> 

Re: [PATCH v2] nvme: consider also host_iface when checking ip options

[Daniel Wagner]

On Wed, Jul 27, 2022 at 02:52:25PM +0800, Chao Leng wrote:
> host_iface is different with traddr and trsvcid.
> traddr is required opt, trsvcid can be specified or the default.
> host_iface is NULL if without specified. It is same with host_tradr.
> 
> For example:
> nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420 -f eth0
> nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420
> According to your patch, the second will be treated as a duplicate.

Correct. And incidentally this is a duplicate :)

> It is similar like this:
> nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420 -w 192.168.154.50
> nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420
> This will be treated as a different new.

Yes, though if in this case eth0 has only IP 192.168.154.50, it would be
a duplicate.

> host_iface and host_tradr are similar opts related to host outgoing.
> host_iface and host_tradr may or may not be specified.
> The check logic should be consistent.

I see. The comment nvmf_ip_options_match() on this topic says the whole
logic is a bit 'rough'. Bringing iface_host into this picture makes the
whole logic even more complex. it's possible to use iface_host and
host_traddr combined. That's why I thought it would be a good idea to
make it more restrict.

I suspect the correct solution would be to inference each option set
which actual configuration is used. Not sure if it is worth the
hassle. Is there any real harm with duplicate connections, except
performance?

Even with this patch, it's possible to do something like this here:

# nvme list-subsys
nvme-subsys0 - NQN=nvmet-test
\
 +- nvme0 tcp traddr=192.168.19.54,trsvcid=4420,host_iface=eth0 live optimized
 +- nvme1 tcp traddr=192.168.19.54,trsvcid=4420,host_traddr=192.168.154.50 live optimized

It is a duplicate connection...

I don't mind relaxing the check but the question is should it combined
with the host_traddr logic or stay separate?

Re: [PATCH v2] nvme: consider also host_iface when checking ip options

[Chao Leng]

On 2022/7/27 16:03, Daniel Wagner wrote:
> On Wed, Jul 27, 2022 at 02:52:25PM +0800, Chao Leng wrote:
>> host_iface is different with traddr and trsvcid.
>> traddr is required opt, trsvcid can be specified or the default.
>> host_iface is NULL if without specified. It is same with host_tradr.
>>
>> For example:
>> nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420 -f eth0
>> nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420
>> According to your patch, the second will be treated as a duplicate.
> 
> Correct. And incidentally this is a duplicate :)
> 
>> It is similar like this:
>> nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420 -w 192.168.154.50
>> nvme connect -t tcp -n nvmet-test -a 192.168.19.54 -s 4420
>> This will be treated as a different new.
> 
> Yes, though if in this case eth0 has only IP 192.168.154.50, it would be
> a duplicate.
> 
>> host_iface and host_tradr are similar opts related to host outgoing.
>> host_iface and host_tradr may or may not be specified.
>> The check logic should be consistent.
> 
> I see. The comment nvmf_ip_options_match() on this topic says the whole
> logic is a bit 'rough'. Bringing iface_host into this picture makes the
> whole logic even more complex. it's possible to use iface_host and
> host_traddr combined. That's why I thought it would be a good idea to
> make it more restrict.
> 
> I suspect the correct solution would be to inference each option set
> which actual configuration is used. Not sure if it is worth the
> hassle. Is there any real harm with duplicate connections, except
> performance? >
> Even with this patch, it's possible to do something like this here:
> 
> # nvme list-subsys
> nvme-subsys0 - NQN=nvmet-test
> \
>   +- nvme0 tcp traddr=192.168.19.54,trsvcid=4420,host_iface=eth0 live optimized
>   +- nvme1 tcp traddr=192.168.19.54,trsvcid=4420,host_traddr=192.168.154.50 live optimized
> 
> It is a duplicate connection...
> 
> I don't mind relaxing the check but the question is should it combined
> with the host_traddr logic or stay separate?
It's really troublesome to make strict judgments. Unexpected repeated connections
in special cases should be acceptable, because the harm is almost negligible and
the probability of actual occurrence is very low.
A bit more strict or relaxing both can be acceptable. but it should be same
for the host_traddr and the host_iface.
> .
> 

Re: [PATCH v2] nvme: consider also host_iface when checking ip options

[Daniel Wagner]

On Fri, Jul 22, 2022 at 11:24:56AM +0200, Daniel Wagner wrote:
> It's perfectly fine to use the same traddr and trsvcid more than once
> as long we use different a host_iface. This is used in setups where
> the host has more than one interface but the target exposes only one
> IP.
> 
> To keep the match logic simple don't allow mixed setups. The user
> needs to use host_iface option consistently for all controllers in
> such setups.
> 
> Signed-off-by: Daniel Wagner <dwagner@suse.de>

Please ignore this patch. The follow up patch is here: 20220722100335.1708-1-dwagner@suse.de