Re: [PATCH bpf] libbpf: Fix bpf_xdp_query() in old kernels

From: Yonghong Song <yhs@meta.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>, Yonghong Song <yhs@fb.com>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	kernel-team@fb.com, Martin KaFai Lau <martin.lau@kernel.org>,
	Lorenzo Bianconi <lorenzo@kernel.org>
Subject: Re: [PATCH bpf] libbpf: Fix bpf_xdp_query() in old kernels
Date: Mon, 27 Feb 2023 15:44:14 -0800	[thread overview]
Message-ID: <d32044c9-e1f9-f33a-21fc-d595ac349cc5@meta.com> (raw)
In-Reply-To: <CAEf4Bzaqqzxo7fMNxrYXf5VgLVqSR3cOGkM6KF=hTNqcc1DTBw@mail.gmail.com>

On 2/27/23 3:35 PM, Andrii Nakryiko wrote:
> On Mon, Feb 27, 2023 at 2:50 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> Commit 04d58f1b26a4("libbpf: add API to get XDP/XSK supported features")
>> added feature_flags to struct bpf_xdp_query_opts. If a user uses
>> bpf_xdp_query_opts with feature_flags member, the bpf_xdp_query()
>> will check whether 'netdev' family exists or not in the kernel.
>> If it does not exist, the bpf_xdp_query() will return -ENOENT.
>>
>> But 'netdev' family does not exist in old kernels as it is
>> introduced in the same patch set as Commit 04d58f1b26a4.
>> So old kernel with newer libbpf won't work properly with
>> bpf_xdp_query() api call.
>>
>> To fix this issue, if the return value of
>> libbpf_netlink_resolve_genl_family_id() is -ENOENT, bpf_xdp_query()
>> will just return 0, skipping the rest of xdp feature query.
>> This preserves backward compatibility.
>>
>> Fixes: 04d58f1b26a4 ("libbpf: add API to get XDP/XSK supported features")
>> Cc: Lorenzo Bianconi <lorenzo@kernel.org>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   tools/lib/bpf/netlink.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/lib/bpf/netlink.c b/tools/lib/bpf/netlink.c
>> index 1653e7a8b0a1..4c1b3502f88d 100644
>> --- a/tools/lib/bpf/netlink.c
>> +++ b/tools/lib/bpf/netlink.c
>> @@ -468,8 +468,11 @@ int bpf_xdp_query(int ifindex, int xdp_flags, struct bpf_xdp_query_opts *opts)
>>                  return 0;
>>
>>          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);
>> +       }
>>
> 
> As I mentioned in another thread, I'm a bit worried of this early
> return, because query_opts might be extended and then we'll forget
> about this early return. So I did these changes and pushed to
> bpf-next:
> 
> diff --git a/tools/lib/bpf/netlink.c b/tools/lib/bpf/netlink.c
> index 4c1b3502f88d..84dd5fa14905 100644
> --- a/tools/lib/bpf/netlink.c
> +++ b/tools/lib/bpf/netlink.c
> @@ -469,8 +469,10 @@ int bpf_xdp_query(int ifindex, int xdp_flags,
> struct bpf_xdp_query_opts *opts)
> 
>          err = libbpf_netlink_resolve_genl_family_id("netdev",
> sizeof("netdev"), &id);
>          if (err < 0) {
> -               if (err == -ENOENT)
> -                       return 0;
> +               if (err == -ENOENT) {
> +                       opts->feature_flags = 0;
> +                       goto skip_feature_flags;
> +               }
>                  return libbpf_err(err);
>          }
> 
> @@ -492,6 +494,7 @@ int bpf_xdp_query(int ifindex, int xdp_flags,
> struct bpf_xdp_query_opts *opts)
> 
>          opts->feature_flags = md.flags;
> 
> +skip_feature_flags:
>          return 0;
>   }

Sounds good to me. Thanks!

> 
>>          memset(&req, 0, sizeof(req));
>>          req.nh.nlmsg_len = NLMSG_LENGTH(GENL_HDRLEN);
>> --
>> 2.30.2
>>