netdev - Re: [Patch net v3] mlx5: force CHECKSUM

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]

Message-ID: <CAM_iQpVW8fRjdTJ32mzuSqAO31gij2NgVynis=ps9540AhW+_Q@mail.gmail.com>
Date:   Tue, 4 Dec 2018 18:48:21 -0800
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Saeed Mahameed <saeedm@...lanox.com>
Cc:     saeedm@....mellanox.co.il,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Tariq Toukan <tariqt@...lanox.com>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: [Patch net v3] mlx5: force CHECKSUM_NONE for short ethernet frames

On Tue, Dec 4, 2018 at 4:59 PM Saeed Mahameed <saeedm@...lanox.com> wrote:
>
> Cong, you are missing some important details, hardware can only parse a
> handful of protocols IP/TCP/UDP some tunneling like vxlan,GRE, etc..
> but for complicated protocols and new generic tunneling protocols,
> which the hardware wasn't built to understand, the checksum complete
> comes to the rescue.
>
> not only that, imagine you are doing some proprietary tunneling and you
> want to encapsulate the rx traffic and send it back to wire, how would
> you want to recalculate the csum before txing ? manually on the whole
> new packet or use the csum complete and just figure out the differences
> ? i am sure you gonna want to use the checksum complete of the original
> packet.
>
> So again csum complete is not only for validation, it is more powerful
> than that.

OK, so how do you detect this? Clearly not by just checking skb->len,
right?

Let me show you why your arguments are non-sense.

1. From your description of how mlx5 hardware works, the driver logic
must be something like this:

if (hardware can checksum L4)
    use CHECKSUM_UNNECESSARY
else
    use CHECKSUM_COMPLETE

This obviously makes sense. Does the current code base match this?
No. The current code base only checks for IP/IPv6 and sets
CHECKSUM_COMPLETE (unless SCTP), far from matching what
you are trying to sell.

2. What you and Eric suggest to change to:

if (the frame is short)
    use CHECKSUM_UNNECESSARY
else
    use CHECKSUM_COMPLETE

Does this match what you describe? No, because short frame does not
necessarily mean hardware can't check its L4 checksum. It only misleads
people to believe so, it helps nothing.

3. What my patch actually does:

if (the frame is short)
    use CHECKSUM_NONE
else
    use CHECKSUM_COMPLETE

Why this makes sense? Because when skb->csum is not correct,
treating it as none is reasonable. And, checking skb->len doesn't
have indication of whether hardware can checksum L4 checksum
or not. This is _good_ when the rest code remains unchanged,
we have no clue whether hardware has done L4 checksum validation
or not, therefore choose to pass it to software to validate.

Of course you can eventually change all the code to match what
you are describing, but _until_ that happens CHECKSUM_NONE
makes more sense than CHECKSUM_UNNECESSARY here.

>
> >
> > > So i agree with Eric, let's jump to checksum_unnecessary for short
> > > packets.
> >
> > This is odd, if Eric is right, then we should completely get rid of
> > CHECKSUM_COMPLETE. Short packets are not exceptions.
> >
>
> short packets are exception,
> 1. because of the small packet padding issue

Why padding issue makes it CHECKSUM_UNNECESSARY not
CHECKSUM_NONE? Isn't the hardware's capability what makes the
difference?

> 2. because they are short enough not to feel the performance hit of
> recalculating their whole csum (if necessary) .

With CHECKSUM_UNNECESSARY, checksum is not recalculated
or validated at L4.

>
> Again still it is nice to report csum unnecessary for those packets, if
> possible.

Please define "if possible". You are hiding a lot of things behind
"if possible" to favor your own argument.

>
> for large packets csum complete is favorable because of the above
> reasons.

Only if hardware can checksum all large packets.

>
>
> if you don't like checksum complete you have a way to totally disable
> it in mlx5 via ethtool private flags.

Thanks but we are very happy to carry the fix locally.

This is why I stop arguing with this non-sense. Why not we all stop
here and agree to an disagreement? Why are we wasting our time?

Thanks!