[PATCH] ovs: Only clear tstamp when changing namespaces

[PATCH] ovs: Only clear tstamp when changing namespaces

[Tyler J. Stachecki]

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 <stachecki.tyler@gmail.com>
---
 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

Re: [PATCH] ovs: Only clear tstamp when changing namespaces

[Cong Wang]

On Sun, Sep 19, 2021 at 10:59 AM Tyler J. Stachecki
<stachecki.tyler@gmail.com> 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?

>
> Signed-off-by: Tyler J. Stachecki <stachecki.tyler@gmail.com>
> ---
>  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.

Re: [PATCH] ovs: Only clear tstamp when changing namespaces

[Tyler Stachecki]

On Sun, Sep 19, 2021 at 7:33 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Sun, Sep 19, 2021 at 10:59 AM Tyler J. Stachecki
> <stachecki.tyler@gmail.com> 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?
>
> >
> > Signed-off-by: Tyler J. Stachecki <stachecki.tyler@gmail.com>
> > ---
> >  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

Re: [PATCH] ovs: Only clear tstamp when changing namespaces

[Cong Wang]

On Sun, Sep 19, 2021 at 10:44 PM Tyler Stachecki
<stachecki.tyler@gmail.com> 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.