On Tue, Aug 04, 2020 at 12:47:56PM -0700, David Miller wrote:
> From: Hangbin Liu <liuhangbin@xxxxxxxxx>
> 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@xxxxxxxxx>
>
> 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
