netdev - Re: [PATCH v5 bpf-next 5/8] libbpf: add API to get XDP/XSK supported features

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: <6b008033-9c97-7f46-310f-1b1ad74a8af6@meta.com>
Date:   Mon, 27 Feb 2023 14:05:21 -0800
From:   Yonghong Song <yhs@...a.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Lorenzo Bianconi <lorenzo@...nel.org>, bpf@...r.kernel.org,
        netdev@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
        andrii@...nel.org, kuba@...nel.org,
        Martin KaFai Lau <martin.lau@...nel.org>
Subject: Re: [PATCH v5 bpf-next 5/8] libbpf: add API to get XDP/XSK supported
 features



On 2/27/23 1:01 PM, Andrii Nakryiko wrote:
> On Mon, Feb 27, 2023 at 12:39 PM Yonghong Song <yhs@...a.com> wrote:
>>
>>
>>
>> On 2/1/23 2:24 AM, Lorenzo Bianconi wrote:
>>> Extend bpf_xdp_query routine in order to get XDP/XSK supported features
>>> of netdev over route netlink interface.
>>> Extend libbpf netlink implementation in order to support netlink_generic
>>> protocol.
>>>
>>> Co-developed-by: Kumar Kartikeya Dwivedi <memxor@...il.com>
>>> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@...il.com>
>>> Co-developed-by: Marek Majtyka <alardam@...il.com>
>>> Signed-off-by: Marek Majtyka <alardam@...il.com>
>>> Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
>>> ---
>>>    tools/lib/bpf/libbpf.h  |  3 +-
>>>    tools/lib/bpf/netlink.c | 96 +++++++++++++++++++++++++++++++++++++++++
>>>    tools/lib/bpf/nlattr.h  | 12 ++++++
>>>    3 files changed, 110 insertions(+), 1 deletion(-)
>>>
>> [...]
>>> +
>>>    int bpf_xdp_query(int ifindex, int xdp_flags, struct bpf_xdp_query_opts *opts)
>>>    {
>>>        struct libbpf_nla_req req = {
>>> @@ -366,6 +433,10 @@ int bpf_xdp_query(int ifindex, int xdp_flags, struct bpf_xdp_query_opts *opts)
>>>                .ifinfo.ifi_family = AF_PACKET,
>>>        };
>>>        struct xdp_id_md xdp_id = {};
>>> +     struct xdp_features_md md = {
>>> +             .ifindex = ifindex,
>>> +     };
>>> +     __u16 id;
>>>        int err;
>>>
>>>        if (!OPTS_VALID(opts, bpf_xdp_query_opts))
>>> @@ -393,6 +464,31 @@ int bpf_xdp_query(int ifindex, int xdp_flags, struct bpf_xdp_query_opts *opts)
>>>        OPTS_SET(opts, skb_prog_id, xdp_id.info.skb_prog_id);
>>>        OPTS_SET(opts, attach_mode, xdp_id.info.attach_mode);
>>>
>>> +     if (!OPTS_HAS(opts, feature_flags))
>>> +             return 0;
>>> +
>>> +     err = libbpf_netlink_resolve_genl_family_id("netdev", sizeof("netdev"), &id);
>>> +     if (err < 0)
>>> +             return libbpf_err(err);
>>
>> Hi, Lorenzo,
>>
>> Using latest libbpf repo (https://github.com/libbpf/libbpf, sync'ed from
>> source), looks like the above change won't work if the program is
>> running on an old kernel, e.g., 5.12 kernel.
>>
>> In this particular combination, in user space, bpf_xdp_query_opts does
>> have 'feature_flags' member, so the control can reach
>> libbpf_netlink_resolve_genl_family_id(). However, the family 'netdev'
>> is only available in latest kernel (after this patch set). So
>> the error will return in the above.
>>
>> This breaks backward compatibility since old working application won't
>> work any more with a refresh of libbpf.
>>
>> I could not come up with an easy solution for this. One thing we could
>> do is to treat 'libbpf_netlink_resolve_genl_family_id()' as a probe, so
>> return 0 if probe fails.
>>
>>     err = libbpf_netlink_resolve_genl_family_id("netdev",
>> sizeof("netdev"), &id);
>>     if (err < 0)
>>          return 0;
>>
>> Please let me know whether my suggestion makes sense or there could be a
>> better solution.
>>
> 
> feature_flags is an output parameter and if the "netdev" family
> doesn't exist then there are no feature flags to return, right?
> 
> Is there a specific error code that's returned when such a family
> doesn't exist? If yes, we should check for it and return 0 for
> feature_flags. If not, we'll have to do a generic < 0 check as
> Yonghong proposes.

We can check -ENOENT.

         err = libbpf_netlink_resolve_genl_family_id("netdev", 
sizeof("netdev"), &id);
-       if (err < 0)
+       if (err < 0) {
+               if (err == -ENOENT)
+                       return 0;
                 return libbpf_err(err);
+       }

Let me propose a patch for this.

> 
> 
>>
>>> +
>>> +     memset(&req, 0, sizeof(req));
>>> +     req.nh.nlmsg_len = NLMSG_LENGTH(GENL_HDRLEN);
>>> +     req.nh.nlmsg_flags = NLM_F_REQUEST;
>>> +     req.nh.nlmsg_type = id;
>>> +     req.gnl.cmd = NETDEV_CMD_DEV_GET;
>>> +     req.gnl.version = 2;
>>> +
>>> +     err = nlattr_add(&req, NETDEV_A_DEV_IFINDEX, &ifindex, sizeof(ifindex));
>>> +     if (err < 0)
>>> +             return err;
>>> +
>>> +     err = libbpf_netlink_send_recv(&req, NETLINK_GENERIC,
>>> +                                    parse_xdp_features, NULL, &md);
>>> +     if (err)
>>> +             return libbpf_err(err);
>>> +
>>> +     opts->feature_flags = md.flags;
>>> +
>>>        return 0;
>>>    }
>>>
>> [...]