netdev - Re: [xdp-hints] Re: [PATCH bpf-next V1] xdp: bpf_xdp_metadata use NODEV for no device 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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 16 Feb 2023 12:33:22 +0100
From:   Alexander Lobakin <aleksander.lobakin@...el.com>
To:     Jesper Dangaard Brouer <jbrouer@...hat.com>
CC:     <brouer@...hat.com>, Larysa Zaremba <larysa.zaremba@...el.com>,
        <bpf@...r.kernel.org>, <netdev@...r.kernel.org>,
        Stanislav Fomichev <sdf@...gle.com>, <martin.lau@...nel.org>,
        <ast@...nel.org>, <daniel@...earbox.net>,
        <alexandr.lobakin@...el.com>, <xdp-hints@...-project.net>
Subject: Re: [xdp-hints] Re: [PATCH bpf-next V1] xdp: bpf_xdp_metadata use
 NODEV for no device support

From: Jesper Dangaard Brouer <jbrouer@...hat.com>
Date: Wed, 15 Feb 2023 18:50:10 +0100

> 
> On 15/02/2023 18.11, Alexander Lobakin wrote:
>> From: Zaremba, Larysa <larysa.zaremba@...el.com>
>> Date: Wed, 15 Feb 2023 16:45:18 +0100

[...]

>>> I think it diverts ENODEV usage from its original purpose too much. 
> 
> Can you suggest a errno that is a better fit?
> 
>>> Maybe providing information in dmesg would be a better solution?
> 
> IMHO we really don't want to print any information in this code path, as
> this is being executed as part of the BPF-prog. This will lead to
> unfortunate latency issues.  Also considering the packet rates this need
> to operate at.
> 
>>
>> +1, -%ENODEV shouldn't be used here. It stands for "no device", for
>> example the driver probing core doesn't treat it as an error or that
>> something is not supported (rather than there's no device installed
>> in a slot / on a bus etc.).
>>
> 
> I wanted to choose something that isn't natural for a device driver
> developer to choose as a return code.  I choose the "no device", because
> the "device" driver doesn't implement this.
> 
> The important part is help ourselves (and support) to reliably determine
> if a device driver implements this kfunc or not. I'm not married to the
> specific errno.
> 
> I hit this issue myself, when developing these kfuncs for igc.  I was
> constantly loading and unloading the driver while developing this. And
> my kfunc would return -EOPNOTSUPP in some cases, and I couldn't
> understand why my code changes was not working, but in reality I was
> hitting the default kfunc implementation as it wasn't the correct
> version of the driver I had loaded.  It would in practice have save me
> time while developing...

So you suggest to pick the properly wrong errno only to make the life of
developers who messed up something in their code a bit easier? I see no
issues with using -%EOPNOTSUPP in every case when the driver can't
provide BPF prog with the hints requested by it.
What you suggest is basically something that we usually do locally to
test WIP stuff at early stages.

> 
> Please suggest a better errno if the color is important to you.
> 
>>>
>>>>
>>>> This is intended to ease supporting and troubleshooting setups. E.g.
>>>> when users on mailing list report -19 (ENODEV) as an error, then we can
>>>> immediately tell them their kernel is too old.
>>>
>>> Do you mean driver being too old, not kernel?
> 
> Sure I guess, I do mean the driver version.
> 
> I guess you are thinking in the lines of Intel customers compiling Intel
> out-of-tree kernel modules, this will also be practical and ease
> troubleshooting for Intel support teams.

The last thing our team thinks of is the Intel customers using
out-of-tree drivers xD

> 
>>>>
>>>> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
>>>> ---
>> [...]
>>
>> Thanks,
>> Olek
>>
>

Thanks,
Olek