netdev - Re: [PATCH bpf-next V1 1/7] xdp: bpf_xdp_metadata use EOPNOTSUPP for no driver 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]

Message-ID: <f42ff647-11b2-4f09-7652-ad85d35b5617@redhat.com>
Date:   Mon, 20 Mar 2023 19:42:22 +0100
From:   Jesper Dangaard Brouer <jbrouer@...hat.com>
To:     Stanislav Fomichev <sdf@...gle.com>
Cc:     brouer@...hat.com, bpf@...r.kernel.org, netdev@...r.kernel.org,
        martin.lau@...nel.org, ast@...nel.org, daniel@...earbox.net,
        alexandr.lobakin@...el.com, larysa.zaremba@...el.com,
        xdp-hints@...-project.net, anthony.l.nguyen@...el.com,
        yoong.siang.song@...el.com, boon.leong.ong@...el.com
Subject: Re: [PATCH bpf-next V1 1/7] xdp: bpf_xdp_metadata use EOPNOTSUPP for
 no driver support


On 17/03/2023 22.21, Stanislav Fomichev wrote:
> On 03/17, Jesper Dangaard Brouer wrote:
>> When driver doesn't implement a bpf_xdp_metadata kfunc the fallback
>> implementation returns EOPNOTSUPP, which indicate device driver doesn't
>> implement this kfunc.
> 
>> Currently many drivers also return EOPNOTSUPP when the hint isn't
>> available, which is inconsistent from an API point of view. Instead
>> change drivers to return ENODATA in these cases.
> 
>> There can be natural cases why a driver doesn't provide any hardware
>> info for a specific hint, even on a frame to frame basis (e.g. PTP).
>> Lets keep these cases as separate return codes.
> 
>> When describing the return values, adjust the function kernel-doc layout
>> to get proper rendering for the return values.
> 
>> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
> 
> I don't remember whether the previous discussion ended in something?
> IIRC Martin was preferring to use xdp-features for this instead?
> 

IIRC Martin asked for a second vote/opinion to settle the vote.
The xdp-features use is orthogonal and this patch does not prohibit the
later implementation of xdp-features, to detect if driver doesn't
implement kfuncs via using global vars.  Not applying this patch leaves
the API in an strange inconsistent state, because of an argument that in
the *future* we can use xdp-features to solve *one* of the discussed
use-cases for another return code.
I argued for a practical PTP use-case where not all frames contain the
PTP timestamp.  This patch solve this use-case *now*, so I don't see why
we should stall solving this, because of a "future" feature we might
never get around to implement, which require the user to use global vars.


> Personally I'm fine with having this convention, but I'm not sure how well
> we'll be able to enforce them. (In general, I'm not a fan of userspace
> changing it's behavior based on errno. If it's mostly for
> debugging/development - seems ok)
>

We enforce the API by documenting the return behavior, like below.  If a 
driver violate this, then we will fix the driver code with a fixes tag.

My ask is simply let not have ambiguous return codes.


>> ---
>>   Documentation/networking/xdp-rx-metadata.rst     |    7 +++++--
>>   drivers/net/ethernet/mellanox/mlx4/en_rx.c       |    4 ++--
>>   drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c |    4 ++--
>>   drivers/net/veth.c                               |    4 ++--
>>   net/core/xdp.c                                   |   10 ++++++++--
>>   5 files changed, 19 insertions(+), 10 deletions(-)
> 
[...]
>> diff --git a/net/core/xdp.c b/net/core/xdp.c
>> index 8d3ad315f18d..7133017bcd74 100644
>> --- a/net/core/xdp.c
>> +++ b/net/core/xdp.c
>> @@ -705,7 +705,10 @@ __diag_ignore_all("-Wmissing-prototypes",
>>    * @ctx: XDP context pointer.
>>    * @timestamp: Return value pointer.
>>    *
>> - * Returns 0 on success or ``-errno`` on error.
>> + * Return:
>> + * * Returns 0 on success or ``-errno`` on error.
>> + * * ``-EOPNOTSUPP`` : means device driver does not implement kfunc
>> + * * ``-ENODATA``    : means no RX-timestamp available for this frame
>>    */
>>   __bpf_kfunc int bpf_xdp_metadata_rx_timestamp(const struct xdp_md 
>> *ctx, u64 *timestamp)
>>   {
>> @@ -717,7 +720,10 @@ __bpf_kfunc int 
>> bpf_xdp_metadata_rx_timestamp(const struct xdp_md *ctx, u64 *tim
>>    * @ctx: XDP context pointer.
>>    * @hash: Return value pointer.
>>    *
>> - * Returns 0 on success or ``-errno`` on error.
>> + * Return:
>> + * * Returns 0 on success or ``-errno`` on error.
>> + * * ``-EOPNOTSUPP`` : means device driver doesn't implement kfunc
>> + * * ``-ENODATA``    : means no RX-hash available for this frame
>>    */
>>   __bpf_kfunc int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, 
>> u32 *hash)
>>   {
> 
>