netdev - Re: [PATCH net] net/mlx4_en: 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] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANn89iKPmQ0=zWt69KHWi3+64GdX4S9JBXuQMG+6VR1TwRH_FA@mail.gmail.com>
Date:   Thu, 31 Jan 2019 11:07:46 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     Saeed Mahameed <saeedm@...lanox.com>
Cc:     Tariq Toukan <tariqt@...lanox.com>,
        Eran Ben Elisha <eranbe@...lanox.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net] net/mlx4_en: Force CHECKSUM_NONE for short ethernet frames

On Thu, Jan 31, 2019 at 11:04 AM Saeed Mahameed <saeedm@...lanox.com> wrote:
>
> On Thu, 2019-01-31 at 07:02 -0800, Eric Dumazet wrote:
> > On Thu, Jan 31, 2019 at 5:04 AM Tariq Toukan <tariqt@...lanox.com>
> > wrote:
> > > From: Saeed Mahameed <saeedm@...lanox.com>
> > >
> > > When an ethernet frame is padded to meet the minimum ethernet frame
> > > size, the padding octets are not covered by the hardware checksum.
> > > Fortunately the padding octets are usually zero's, which don't
> > > affect
> > > checksum. However, it is not guaranteed. For example, switches
> > > might
> > > choose to make other use of these octets.
> > > This repeatedly causes kernel hardware checksum fault.
> > >
> > > Prior to the cited commit below, skb checksum was forced to be
> > > CHECKSUM_NONE when padding is detected. After it, we need to keep
> > > skb->csum updated. However, fixing up CHECKSUM_COMPLETE requires to
> > > verify and parse IP headers, it does not worth the effort as the
> > > packets
> > > are so small that CHECKSUM_COMPLETE has no significant advantage.
> > >
> > > Fixes: 88078d98d1bb ("net: pskb_trim_rcsum() and CHECKSUM_COMPLETE
> > > are friends")
> > > Cc: Eric Dumazet <edumazet@...gle.com>
> > > Signed-off-by: Saeed Mahameed <saeedm@...lanox.com>
> > > Signed-off-by: Tariq Toukan <tariqt@...lanox.com>
> > > ---
> > >  drivers/net/ethernet/mellanox/mlx4/en_rx.c | 19 +++++++++++++++++-
> > > -
> > >  1 file changed, 17 insertions(+), 2 deletions(-)
> > >
> > > Hi Dave,
> > > Please queue for -stable >= v4.18.
> > >
> > > Thanks,
> > > Tariq
> > >
> > > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> > > b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> > > index 9a0881cb7f51..fc8a11444e1a 100644
> > > --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> > > +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> > > @@ -617,6 +617,8 @@ static int get_fixed_ipv6_csum(__wsum
> > > hw_checksum, struct sk_buff *skb,
> > >  }
> > >  #endif
> > >
> > > +#define short_frame(size) ((size) <= ETH_ZLEN + ETH_FCS_LEN)
> > > +
> > >  /* We reach this function only after checking that any of
> > >   * the (IPv4 | IPv6) bits are set in cqe->status.
> > >   */
> > > @@ -624,9 +626,22 @@ static int check_csum(struct mlx4_cqe *cqe,
> > > struct sk_buff *skb, void *va,
> > >                       netdev_features_t dev_features)
> > >  {
> > >         __wsum hw_checksum = 0;
> > > +       void *hdr;
> > > +
> > > +       /* CQE csum doesn't cover padding octets in short ethernet
> > > +        * frames. And the pad field is appended prior to
> > > calculating
> > > +        * and appending the FCS field.
> > > +        *
> > > +        * Detecting these padded frames requires to verify and
> > > parse
> > > +        * IP headers, so we simply force all those small frames to
> > > be
> > > +        * CHECKSUM_NONE even if they are not padded.
> > > +        * TODO: better if we report CHECKSUM_UNNECESSARY but this
> > > +        * demands some refactroing.
> >
> > This TODO: looks bogus to me.
> >
> > mlx4 driver first tries to use CHECKSUM_UNNECESSARY.
> >
> > if ((cqe->status & cpu_to_be16(MLX4_CQE_STATUS_TCP |
> >                                                   MLX4_CQE_STATUS_UDP
> > )) &&
> >      (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPOK)) &&
> >       cqe->checksum == cpu_to_be16(0xffff)) {
> >      ...
> >       ip_summed = CHECKSUM_UNNECESSARY;
> >       ...
> > }
> >
> > Then if this attempt failed, it tries to use CHECKSUM_COMPLETE (and
> > calls this check_sum() function)
> >
> > So there is no refactoring to be done for mlx4 : short
> > IPv[46]+{UDP|TCP} frames get CHECKSUM_UNNECESSARY already
>
> not exactly, considering mlx4 weird condition to decide to do
> CHECKSUM_UNNECESSARY:
>
> if ((cqe csum fields are valid) && cqe->checksum ==
> cpu_to_be16(0xffff))
>      { do CHECKSUM_UNNECESSARY }
> else { do CHECKSUM_COMPLETE }
>
> CHECKSUM_UNNECESSARY will be skipped if csum complete field is valid
> i.e (cqe->checksum != 0xffff)
>
> but if checksum complete is to be skipped due to short_frame we want to
> go back to CHECKSUM_UNNECESSARY, this is the refactoring i am talking
> about :).
>
>
> I hope now it is clear..

Not really, since in case of short frame, cqe->checksum will be
0xffff, since mlx4 does not include the padding bytes in the checksum
in the first place.

(For native IPv4/IPV6 TCP/UDP frames that is)

>
>
> >
> > > +        */
> > > +       if (short_frame(skb->len))
> > > +               return -EINVAL;
> > >
> > > -       void *hdr = (u8 *)va + sizeof(struct ethhdr);
> > > -
> > > +       hdr = (u8 *)va + sizeof(struct ethhdr);
> > >         hw_checksum = csum_unfold((__force __sum16)cqe->checksum);
> > >
> > >         if (cqe->vlan_my_qpn &
> > > cpu_to_be32(MLX4_CQE_CVLAN_PRESENT_MASK) &&
> > > --
> > > 1.8.3.1
> > >