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

[Leon Romanovsky <leon@kernel.org>]

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