[PATCH] ovs: Only clear tstamp when changing namespaces

As of "ovs: clear skb->tstamp in forwarding path", the
tstamp is now being cleared unconditionally to fix fq qdisc
operation with ovs vports.

While this is mostly correct and fixes forwarding for that
use case, a slight adjustment is necessary to ensure that
the tstamp is cleared *only when the forwarding is across
namespaces*.

Signed-off-by: Tyler J. Stachecki <redacted>
---
 net/openvswitch/vport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
index cf2ce5812489..c2d32a5c3697 100644
--- a/net/openvswitch/vport.c
+++ b/net/openvswitch/vport.c

@@ -507,7 +507,8 @@ void ovs_vport_send(struct vport *vport, struct sk_buff *skb, u8 mac_proto)
 	}
 
 	skb->dev = vport->dev;
-	skb->tstamp = 0;
+	if (dev_net(skb->dev))
+		skb->tstamp = 0;
 	vport->ops->send(skb);
 	return;
 

-- 
2.20.1

On Sun, Sep 19, 2021 at 10:59 AM Tyler J. Stachecki
[off-list ref] wrote:

As of "ovs: clear skb->tstamp in forwarding path", the
tstamp is now being cleared unconditionally to fix fq qdisc
operation with ovs vports.

While this is mostly correct and fixes forwarding for that
use case, a slight adjustment is necessary to ensure that
the tstamp is cleared *only when the forwarding is across
namespaces*.

Hmm? I am sure timestamp has already been cleared when
crossing netns:

void skb_scrub_packet(struct sk_buff *skb, bool xnet)
{
...
        if (!xnet)
                return;

        ipvs_reset(skb);
        skb->mark = 0;
        skb->tstamp = 0;
}

So, what are you trying to fix?

Doesn't dev_net() always return a non-NULL pointer?

If you really want to check whether it is cross-netns, you should
use net_eq() to compare src netns with dst netns, something like:
if (!net_eq(dev_net(vport->dev), dev_net(skb->dev))).

Thanks.

On Sun, Sep 19, 2021 at 7:33 PM Cong Wang [off-list ref] wrote:

On Sun, Sep 19, 2021 at 10:59 AM Tyler J. Stachecki
[off-list ref] wrote:

quoted

As of "ovs: clear skb->tstamp in forwarding path", the
tstamp is now being cleared unconditionally to fix fq qdisc
operation with ovs vports.

While this is mostly correct and fixes forwarding for that
use case, a slight adjustment is necessary to ensure that
the tstamp is cleared *only when the forwarding is across
namespaces*.

Hmm? I am sure timestamp has already been cleared when
crossing netns:

void skb_scrub_packet(struct sk_buff *skb, bool xnet)
{
...
        if (!xnet)
                return;

        ipvs_reset(skb);
        skb->mark = 0;
        skb->tstamp = 0;
}

So, what are you trying to fix?

quoted

Signed-off-by: Tyler J. Stachecki <redacted>
---
 net/openvswitch/vport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
index cf2ce5812489..c2d32a5c3697 100644
--- a/net/openvswitch/vport.c
+++ b/net/openvswitch/vport.c

@@ -507,7 +507,8 @@ void ovs_vport_send(struct vport *vport, struct sk_buff *skb, u8 mac_proto)
        }

        skb->dev = vport->dev;
-       skb->tstamp = 0;
+       if (dev_net(skb->dev))

Doesn't dev_net() always return a non-NULL pointer?

If you really want to check whether it is cross-netns, you should
use net_eq() to compare src netns with dst netns, something like:
if (!net_eq(dev_net(vport->dev), dev_net(skb->dev))).

Thanks.

Sorry if this is a no-op -- I'm admittedly not familiar with this part
of the tree.  I had added this check based on this discussion on the
OVS mailing list:
https://mail.openvswitch.org/pipermail/ovs-discuss/2021-February/050966.html

The motivation to add it was based on the fact that skb_scrub_packet
is doing it conditionally as well, but you seem to indicate that
skb_scrub_packet itself is already being done somewhere?

Cheers,
Tyler

On Sun, Sep 19, 2021 at 10:44 PM Tyler Stachecki
[off-list ref] wrote:

Sorry if this is a no-op -- I'm admittedly not familiar with this part
of the tree.  I had added this check based on this discussion on the
OVS mailing list:
https://mail.openvswitch.org/pipermail/ovs-discuss/2021-February/050966.html

The motivation to add it was based on the fact that skb_scrub_packet
is doing it conditionally as well, but you seem to indicate that
skb_scrub_packet itself is already being done somewhere?

I mean, skb->tstamp has been cleared when crossing netns,
so: 1) you don't need to clear it again for this case; 2) clearly we
fix other cases with commit 01634047bf0d, so your patch break
it again.

Thanks.