netdev - Re: [PATCH v6 net-next 3/9] net: ethtool: record custom RSS contexts in the XArray

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]

Date: Tue, 25 Jun 2024 14:39:23 +0100
From: Edward Cree <ecree.xilinx@...il.com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>, edward.cree@....com
Cc: linux-net-drivers@....com, netdev@...r.kernel.org,
 habetsm.xilinx@...il.com, sudheer.mogilappagari@...el.com,
 jdamato@...tly.com, mw@...ihalf.com, linux@...linux.org.uk,
 sgoutham@...vell.com, gakula@...vell.com, sbhatta@...vell.com,
 hkelam@...vell.com, saeedm@...dia.com, leon@...nel.org,
 jacob.e.keller@...el.com, andrew@...n.ch, ahmed.zaki@...el.com,
 davem@...emloft.net, kuba@...nel.org, edumazet@...gle.com, pabeni@...hat.com
Subject: Re: [PATCH v6 net-next 3/9] net: ethtool: record custom RSS contexts
 in the XArray

On 20/06/2024 07:32, Przemek Kitszel wrote:
> On 6/20/24 07:47, edward.cree@....com wrote:
>> +    return struct_size((struct ethtool_rxfh_context *)0, data, flex_len);
> 
> struct_size_t

Yup, will do, thanks for the suggestion.
Don't think that existed yet when I wrote v1 :-D

>> +    /* Update rss_ctx tracking */
>> +    if (create) {
>> +        /* Ideally this should happen before calling the driver,
>> +         * so that we can fail more cleanly; but we don't have the
>> +         * context ID until the driver picks it, so we have to
>> +         * wait until after.
>> +         */
>> +        if (WARN_ON(xa_load(&dev->ethtool->rss_ctx, rxfh.rss_context))) {
>> +            /* context ID reused, our tracking is screwed */
> 
> why no error code set?

Because at this point the driver *has* created the context, it's
 in the hardware.  If we wanted to return failure we'd have to
 call the driver again to delete it, and that would still leave
 an ugly case where that call fails.

> 
>> +            kfree(ctx);
>> +            goto out;
>> +        }
>> +        /* Allocate the exact ID the driver gave us */
>> +        if (xa_is_err(xa_store(&dev->ethtool->rss_ctx, rxfh.rss_context,
>> +                       ctx, GFP_KERNEL))) {
> 
> this is racy - assuming it is possible that context was set by other
> means (otherwisce you would not xa_load() a few lines above) -
> a concurrent writer could have done this just after you xa_load() call.

I don't expect a concurrent writer - this is all under RTNL.
The xa_load() is there in case we create two contexts
 consecutively and the driver gives us the same ID both times.

> so, instead of xa_load() + xa_store() just use xa_insert()

The reason for splitting it up is for the WARN_ON on the
 xa_load().  I guess with xa_insert() it would have to be
 WARN_ON(xa_insert() == -EBUSY)?

> anyway I feel the pain of trying to support both driver-selected IDs
> and your own