netdev - Re: [PATCH net-next v7] ethtool: add netlink based get rss support

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: <Y5GesjGvi+Gtj+Dq@unreal>
Date:   Thu, 8 Dec 2022 10:22:10 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     Michal Kubecek <mkubecek@...e.cz>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        Sudheer Mogilappagari <sudheer.mogilappagari@...el.com>,
        netdev@...r.kernel.org, andrew@...n.ch, corbet@....net,
        sridhar.samudrala@...el.com, anthony.l.nguyen@...el.com
Subject: Re: [PATCH net-next v7] ethtool: add netlink based get rss support

On Wed, Dec 07, 2022 at 10:32:48AM +0100, Michal Kubecek wrote:
> On Wed, Dec 07, 2022 at 10:42:38AM +0200, Leon Romanovsky wrote:
> > On Tue, Dec 06, 2022 at 05:14:41PM +0100, Michal Kubecek wrote:
> > > 
> > >   - avoiding the inherently racy get/modify/set cycle
> > 
> > How? IMHO, it is achieved in netlink by holding relevant locks, it can
> > be rtnl lock or specific to that netlink interface lock (devl). You cam
> > and should have same locking protection for legacy flow as well.
> 
> What I had in mind is changing only one (or few) of the parameters which
> are passed in a structure via ioctl interface, i.e. commands like
> 
>   ethtool -G eth0 rx 2048
> 
> To do that with ioctl interface, userspace needs to fetch the whole
> ethtool_ringparam structure with ETHTOOL_GRINGPARAM first, modify its
> rx_pending member and pass the structure back with ETHTOOL_SRINGPARAM.
> Obviously you cannot hold a kernel lock over multiple ioctl() syscall.

Kernel historically doesn't have protection from user space races,
which is what you presented here. Netlink gives you feature flags
over specific fields, which you can achieve over ioctl too.

Anyway, I see your point.

> 
> In some cases, there is a special with "no change" meaning but that is
> rather an exception. It would be possible to work around the problem
> using some "version counter" that would kernel check against its own
> (and reject the update if they do not match) but introducing that would
> also be a backward incompatible change.

Another options is to build netlink over ioctl. See as an example RDMA ioctl
interface (ib_uverbs_ioctl ...) which in nutshell is netlink over ioctl. After
long debates, we choose ioctl, because of need to have synchronous and reliable
interface to configure system. But we liked TLV structure of netlink, so used it.

Thanks

> 
> Michal