[PATCH] Revert "RDMA/rxe: Remove VLAN code leftovers from RXE"

[PATCH] Revert "RDMA/rxe: Remove VLAN code leftovers from RXE"

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This reverts commit b2d2440430c0fdd5e0cad3efd6d1c9e3d3d02e5b.

It's true that creating rxe on top of 802.1q interfaces doesn't work.
Thus, commit fd49ddaf7e26 ("RDMA/rxe: prevent rxe creation on top of vlan interface")
was absolutely correct.

But b2d2440430c0 was incorrect assuming that with this change,
RDMA and VLAN don't work togehter at all. It just has to be
set up differently. Rather than creating rxe on top of the VLAN
interface, rxe must be created on top of the physical interface.
RDMA then works just fine through VLAN interfaces on top of that
physical interface, via the "upper device" logic.

I've tested this mainly with NVMe over RDMA and rping, but I don't
see why it wouldn't work just as well for other protocols. If there
are real issues, I'd like to know.

b2d2440430c0 broke this setup deliberately and should thus be
reverted.

Fixes: b2d2440430c0 ("RDMA/rxe: Remove VLAN code leftovers from RXE")

Cc: Zhu Yanjun <zyjzyj2000@gmail.com>
Cc: Mohammad Heib <goody698@gmail.com>
Cc: Vijay Immanuel <vijayi@attalasystems.com>
Cc: Nicolas Morey-Chaisemartin <nmoreychaisemartin@suse.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>

---
Note: I'm currently not subscribed to linux-rdma.

---
 drivers/infiniband/sw/rxe/rxe_net.c  | 18 ++++++++++++++++++
 drivers/infiniband/sw/rxe/rxe_resp.c |  5 +++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
index c4b06ced30a7..34bef7d8e6b4 100644
--- a/drivers/infiniband/sw/rxe/rxe_net.c
+++ b/drivers/infiniband/sw/rxe/rxe_net.c
@@ -8,6 +8,7 @@
 #include <linux/if_arp.h>
 #include <linux/netdevice.h>
 #include <linux/if.h>
+#include <linux/if_vlan.h>
 #include <net/udp_tunnel.h>
 #include <net/sch_generic.h>
 #include <linux/netfilter.h>
@@ -19,6 +20,18 @@
 
 static struct rxe_recv_sockets recv_sockets;
 
+struct device *rxe_dma_device(struct rxe_dev *rxe)
+{
+	struct net_device *ndev;
+
+	ndev = rxe->ndev;
+
+	if (is_vlan_dev(ndev))
+		ndev = vlan_dev_real_dev(ndev);
+
+	return ndev->dev.parent;
+}
+
 int rxe_mcast_add(struct rxe_dev *rxe, union ib_gid *mgid)
 {
 	int err;
@@ -153,9 +166,14 @@ static int rxe_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 {
 	struct udphdr *udph;
 	struct net_device *ndev = skb->dev;
+	struct net_device *rdev = ndev;
 	struct rxe_dev *rxe = rxe_get_dev_from_net(ndev);
 	struct rxe_pkt_info *pkt = SKB_TO_PKT(skb);
 
+	if (!rxe && is_vlan_dev(rdev)) {
+		rdev = vlan_dev_real_dev(ndev);
+		rxe = rxe_get_dev_from_net(rdev);
+	}
 	if (!rxe)
 		goto drop;
 
diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
index 5a098083a9d2..c7e3b6a4af38 100644
--- a/drivers/infiniband/sw/rxe/rxe_resp.c
+++ b/drivers/infiniband/sw/rxe/rxe_resp.c
@@ -872,6 +872,11 @@ static enum resp_states do_complete(struct rxe_qp *qp,
 			else
 				wc->network_hdr_type = RDMA_NETWORK_IPV6;
 
+			if (is_vlan_dev(skb->dev)) {
+				wc->wc_flags |= IB_WC_WITH_VLAN;
+				wc->vlan_id = vlan_dev_vlan_id(skb->dev);
+			}
+
 			if (pkt->mask & RXE_IMMDT_MASK) {
 				wc->wc_flags |= IB_WC_WITH_IMM;
 				wc->ex.imm_data = immdt_imm(pkt);
-- 
2.29.2

Re: [PATCH] Revert "RDMA/rxe: Remove VLAN code leftovers from RXE"

[Zhu Yanjun]

On Tue, Jan 19, 2021 at 6:57 PM <mwilck@suse.com> wrote:
>
> From: Martin Wilck <mwilck@suse.com>
>
> This reverts commit b2d2440430c0fdd5e0cad3efd6d1c9e3d3d02e5b.
>
> It's true that creating rxe on top of 802.1q interfaces doesn't work.
> Thus, commit fd49ddaf7e26 ("RDMA/rxe: prevent rxe creation on top of vlan interface")
> was absolutely correct.
>
> But b2d2440430c0 was incorrect assuming that with this change,
> RDMA and VLAN don't work togehter at all. It just has to be
> set up differently. Rather than creating rxe on top of the VLAN
> interface, rxe must be created on top of the physical interface.
> RDMA then works just fine through VLAN interfaces on top of that
> physical interface, via the "upper device" logic.

I read this commit log for several times. I can not get you.
Can you show me by an example?

Zhu Yanjun

>
> I've tested this mainly with NVMe over RDMA and rping, but I don't
> see why it wouldn't work just as well for other protocols. If there
> are real issues, I'd like to know.
>
> b2d2440430c0 broke this setup deliberately and should thus be
> reverted.
>
> Fixes: b2d2440430c0 ("RDMA/rxe: Remove VLAN code leftovers from RXE")
>
> Cc: Zhu Yanjun <zyjzyj2000@gmail.com>
> Cc: Mohammad Heib <goody698@gmail.com>
> Cc: Vijay Immanuel <vijayi@attalasystems.com>
> Cc: Nicolas Morey-Chaisemartin <nmoreychaisemartin@suse.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
>
> ---
> Note: I'm currently not subscribed to linux-rdma.
>
> ---
>  drivers/infiniband/sw/rxe/rxe_net.c  | 18 ++++++++++++++++++
>  drivers/infiniband/sw/rxe/rxe_resp.c |  5 +++++
>  2 files changed, 23 insertions(+)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
> index c4b06ced30a7..34bef7d8e6b4 100644
> --- a/drivers/infiniband/sw/rxe/rxe_net.c
> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
> @@ -8,6 +8,7 @@
>  #include <linux/if_arp.h>
>  #include <linux/netdevice.h>
>  #include <linux/if.h>
> +#include <linux/if_vlan.h>
>  #include <net/udp_tunnel.h>
>  #include <net/sch_generic.h>
>  #include <linux/netfilter.h>
> @@ -19,6 +20,18 @@
>
>  static struct rxe_recv_sockets recv_sockets;
>
> +struct device *rxe_dma_device(struct rxe_dev *rxe)
> +{
> +       struct net_device *ndev;
> +
> +       ndev = rxe->ndev;
> +
> +       if (is_vlan_dev(ndev))
> +               ndev = vlan_dev_real_dev(ndev);
> +
> +       return ndev->dev.parent;
> +}
> +
>  int rxe_mcast_add(struct rxe_dev *rxe, union ib_gid *mgid)
>  {
>         int err;
> @@ -153,9 +166,14 @@ static int rxe_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
>  {
>         struct udphdr *udph;
>         struct net_device *ndev = skb->dev;
> +       struct net_device *rdev = ndev;
>         struct rxe_dev *rxe = rxe_get_dev_from_net(ndev);
>         struct rxe_pkt_info *pkt = SKB_TO_PKT(skb);
>
> +       if (!rxe && is_vlan_dev(rdev)) {
> +               rdev = vlan_dev_real_dev(ndev);
> +               rxe = rxe_get_dev_from_net(rdev);
> +       }
>         if (!rxe)
>                 goto drop;
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> index 5a098083a9d2..c7e3b6a4af38 100644
> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> @@ -872,6 +872,11 @@ static enum resp_states do_complete(struct rxe_qp *qp,
>                         else
>                                 wc->network_hdr_type = RDMA_NETWORK_IPV6;
>
> +                       if (is_vlan_dev(skb->dev)) {
> +                               wc->wc_flags |= IB_WC_WITH_VLAN;
> +                               wc->vlan_id = vlan_dev_vlan_id(skb->dev);
> +                       }
> +
>                         if (pkt->mask & RXE_IMMDT_MASK) {
>                                 wc->wc_flags |= IB_WC_WITH_IMM;
>                                 wc->ex.imm_data = immdt_imm(pkt);
> --
> 2.29.2
>

Re: [PATCH] Revert "RDMA/rxe: Remove VLAN code leftovers from RXE"

[Jason Gunthorpe]

On Tue, Jan 19, 2021 at 02:40:52PM +0200, Mohamed._. Heib wrote:
>    Hi,
>    As far as i remember the traffic still can be encapsulated over the
>    vlan interface with vlan tag
>    by specifying the vlan interface gid index that mapped to the vlan ip
>    on the vlan parent interface gids table.
>    And by removing the vlan related functionality on rxe (commit:
>    b2d2440430c0) the rxe still can send traffic via vlan interface
>    but can't receive from vlan interface and that will break the vlan
>    functionality on rxe so i think you need keep the vlan related code on
>    rxe.

That may be, but the code here is still wrong.

Incoming packets are supposed to be matched to the gid table, which
specifies the allowed vlans, it can't just be taken raw from the skb
like this.

If someone wants to add vlan support to rxe it needs to be done right,
currently it doesn't support vlan.

Jason

Re: [PATCH] Revert "RDMA/rxe: Remove VLAN code leftovers from RXE"

[Martin Wilck]

On Tue, 2021-01-19 at 09:01 -0400, Jason Gunthorpe wrote:
> On Tue, Jan 19, 2021 at 02:40:52PM +0200, Mohamed._. Heib wrote:
> >    Hi,
> >    As far as i remember the traffic still can be encapsulated over
> > the
> >    vlan interface with vlan tag
> >    by specifying the vlan interface gid index that mapped to the
> > vlan ip
> >    on the vlan parent interface gids table.
> >    And by removing the vlan related functionality on rxe (commit:
> >    b2d2440430c0) the rxe still can send traffic via vlan interface
> >    but can't receive from vlan interface and that will break the
> > vlan
> >    functionality on rxe so i think you need keep the vlan related
> > code on
> >    rxe.
> 
> That may be, but the code here is still wrong.
> 
> Incoming packets are supposed to be matched to the gid table, which
> specifies the allowed vlans, it can't just be taken raw from the skb
> like this.

I lack the deep understanding of the RDMA stack to fully understand.
Are you saying it's a permission problem? Do you see a security issue?
Or is it not compliant with some spec? Which?

> If someone wants to add vlan support to rxe it needs to be done
> right,
> currently it doesn't support vlan.

Hm. As a matter of fact, it has "worked" this way in mainline since
v5.0, when 43c9fc509fa5 ("rdma_rxe: make rxe work over 802.1q VLAN
devices") had been merged. The code that deals with VLANs in RXE, which
was fixed by that commit, is older. AFAICS it dates back to the first
merge of the rxe driver.

Just ripping the code out causes a regression, after 10 kernel minor
releases during which it "worked" in practice (as far as I can judge).
I suppose there are only a few users of this feature, but I fail to see
why keeping this code in until a clean implementation is available
would do any harm.

I'd like to fix this for good, but I lack the knowledge to do this, at
least in the short term. I've asked back in 2018 how RXE+VLAN was
intended to work, but never got an answer.

Regards,
Martin

Re: [PATCH] Revert "RDMA/rxe: Remove VLAN code leftovers from RXE"

[Martin Wilck]

On Tue, 2021-01-19 at 20:10 +0800, Zhu Yanjun wrote:
> On Tue, Jan 19, 2021 at 6:57 PM <mwilck@suse.com> wrote:
> > 
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > This reverts commit b2d2440430c0fdd5e0cad3efd6d1c9e3d3d02e5b.
> > 
> > It's true that creating rxe on top of 802.1q interfaces doesn't
> > work.
> > Thus, commit fd49ddaf7e26 ("RDMA/rxe: prevent rxe creation on top
> > of vlan interface")
> > was absolutely correct.
> > 
> > But b2d2440430c0 was incorrect assuming that with this change,
> > RDMA and VLAN don't work togehter at all. It just has to be
> > set up differently. Rather than creating rxe on top of the VLAN
> > interface, rxe must be created on top of the physical interface.
> > RDMA then works just fine through VLAN interfaces on top of that
> > physical interface, via the "upper device" logic.
> 
> I read this commit log for several times. I can not get you.
> Can you show me by an example?

My test scenario which is broken by your patch uses a script that does
roughly the following:

# (set up eth0)
rdma link add rxe_eth0 type rxe netdev eth0
ip link add link eth0 name eth0.10 type vlan id 10
ip link set eth0.10 up
ip addr add 192.168.10.102/24 dev eth0.10

nvme discover -t rdma -a 192.168.10.101 -s 4420  

192.168.10.101 is another host that configures the network
and rxe the same way, and has some nvmet targets.

  => fails with your patch applied, works otherwise.

Regards,
Martin