netdev - Re: [PATCHv2 net 2/2] vxlan: fix getting tos value from DSCP field

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 5 Aug 2020 10:20:29 +0800
From:   Hangbin Liu <liuhangbin@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     netdev@...r.kernel.org, gnault@...hat.com, pmachata@...il.com,
        roopa@...ulusnetworks.com, dsahern@...nel.org, akaris@...hat.com,
        stable@...r.kernel.org
Subject: Re: [PATCHv2 net 2/2] vxlan: fix getting tos value from DSCP field

On Tue, Aug 04, 2020 at 12:47:56PM -0700, David Miller wrote:
> From: Hangbin Liu <liuhangbin@...il.com>
> Date: Tue,  4 Aug 2020 09:43:12 +0800
> 
> > In commit 71130f29979c ("vxlan: fix tos value before xmit") we strict
> > the vxlan tos value before xmit. But as IP tos field has been obsoleted
> > by RFC2474, and updated by RFC3168 later. We should use new DSCP field,
> > or we will lost the first 3 bits value when xmit.
> > 
> > Fixes: 71130f29979c ("vxlan: fix tos value before xmit")
> > Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> 
> Looking at the Fixes: tag commit more closely, it doesn't make much
> sense at all to me and I think the fix is that the Fixes: commit
> should be reverted.

Hi David,

Both this patch and the Fixes: commit are not aim to fix the ECN bits.
ECN bits are handled by ip_tunnel_ecn_encap() correctly.

The Fixes: commit and this patch are aim to fix the TOS/DSCP field, just
as the commit subject said.

In my first patch "net: add IP_DSCP_MASK", as I said, the current
RT_TOS()/IPTOS_TOS_MASK will ignore the first 3 bits in IP header
based on RFC1349.

       0     1     2     3     4     5     6     7
    +-----+-----+-----+-----+-----+-----+-----+-----+
    |   PRECEDENCE    |          TOS          | MBZ |
    +-----+-----+-----+-----+-----+-----+-----+-----+

While in RFC3168 we defined DSCP field like

       0     1     2     3     4     5     6     7
    +-----+-----+-----+-----+-----+-----+-----+-----+
    |          DS FIELD, DSCP           | ECN FIELD |
    +-----+-----+-----+-----+-----+-----+-----+-----+

So if a user defined the IP DSCP/TOS field like 1111 1100, with
RT_TOS(tos) we will got tos 0001 1100, but based on RFC3168, we
should send the header with DSCP 1111 1100. That's why I add
RT_DSCP() in my first patch.

> 
> If you pass the raw TOS into ip_tunnel_ecn_encap(), then that has the
> same exact effect as your patch series here.  The ECN encap routines
> will clear the ECN bits before potentially incorporating the ECN value
> from the inner header etc.  The clearing of the ECN bits done by your
> RT_DSCP() helper is completely unnecessary, the ECN helpers do the
> right thing.  So effectively the RT_DSCP() isn't changing the tos
> value at all.

Yes, you are right. RT_DSCP() doesn't change the tos value in this case.
I will revert the Fixes: commit.

While RT_DSCP() is still needed in other place, I will re-post a new patch
set for that issue.

> 
> I also think that your commit messages are lacking, as you fail
> (especially in the Fixes: commit) to show exactly where things go
> wrong.  It's always good to give example code paths and show what
> happens to the TOS and/or ECN values in these places, what part of
> that transformation you feel is incorrect, and what exactly you
> believe the correct transformation to be.

Thanks, I will try add more info in later patches.

Hangbin