netdev - Re: [PATCH net-next v2] docs: net: dsa: describe issues with checksum offload

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: <CAJq09z6XTz7Xb0VBFdFVELb26LztFng7hULe6tSDX7KCQjzUmg@mail.gmail.com>
Date:   Fri, 15 Apr 2022 05:01:46 -0300
From:   Luiz Angelo Daros de Luca <luizluca@...il.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     Andrew Lunn <andrew@...n.ch>, Kurt Kanzenbach <kurt@...utronix.de>,
        George McCollister <george.mccollister@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "corbet@....net" <corbet@....net>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH net-next v2] docs: net: dsa: describe issues with checksum offload

> On Wed, Apr 13, 2022 at 11:48:46PM -0300, Luiz Angelo Daros de Luca wrote:
> > > Ok, I'll go with "no checksum offload for its trailer tag, and bugs
> > > never fixed because no one uses it", in any case no Sasquatch. Thanks.
> >
> > Vladimir, so the DSA switch will not copy the offload flags when a tag
> > requests tail room? At least it will work.
> >
> > Now, if the offload HW does support that tag, what would be the
> > options? Set the slave port checksum flag from userland?
> > It would be nice to have some type of "magic trick" to have it enabled
> > by default. I'm already expecting a no, but wouldn't it be a nice case
> > for a DSA property in the device tree?
> >
> > Regards,
> >
> > Luiz
>
> DSA calls netdev_upper_dev_link(master, slave_dev, NULL) to establish a
> relationship with its master and the master driver can detect this by
> monitoring NETDEV_CHANGEUPPER.
>
> If we look a bit closer at the implementation of netdev_upper_dev_link
> we see it calls __netdev_upper_dev_link() which contains an interesting
> pair of arguments "void *upper_priv, void *upper_info". These are
> accessible to netdev_master_upper_dev_link(), and the bonding driver
> (for example) makes use of them, see bond_master_upper_dev_link().
>
> I'm thinking DSA could create a struct netdev_dsa_upper_info too, and
> certain DSA masters could populate things in it. Then DSA could look at
> what the DSA master has said (or not said) and fix up features
> accordingly.
>
> One information that could get populated by the master is a bit field of
> whether checksumming is supported for a certain tagging protocol.
> I'd rather pass a full bit field of all tagging protocols, rather than
> just the protocol in current use by the slave, because:
> (a) it's less gory compared to having the master look at
>     dsa_port_from_netdev(info->upper_dev)->cpu_dp->tag_ops->proto
> (b) the DSA tagging protocol can change at runtime, and when it does, no
>     NETDEV_CHANGEUPPER will be emitted, so the master won't have a
>     chance to inform us whether it can offload checksumming for the new
>     protocol. So it's better to have this information from the get go.
>
> We'd also need the DSA master to populate a "bool acked=true" from this
> new struct netdev_dsa_upper_info. The reason is to be able to
> distinguish between an empty bit mask that means "yup, I really can't
> offload checksumming for anything", and a bit mask that means "DSA who?"
> (where checksum offloading is expected to work under the normal
> circumstances described by you, no special code required).

That looks like a larger change. Should we put this patch on hold
waiting for the code refactor or we merge it as is (as it tells no
lies about current code).