Toke Høiland-Jørgensen [off-list ref] [Mon, 2020-03-23 04:25 -0700]:

Andrii Nakryiko [off-list ref] writes:

quoted

On Fri, Mar 20, 2020 at 1:48 AM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted

Jakub Kicinski [off-list ref] writes:

quoted

On Thu, 19 Mar 2020 14:13:13 +0100 Toke Høiland-Jørgensen wrote:

quoted

From: Toke Høiland-Jørgensen <redacted>

While it is currently possible for userspace to specify that an existing
XDP program should not be replaced when attaching to an interface, there is
no mechanism to safely replace a specific XDP program with another.

This patch adds a new netlink attribute, IFLA_XDP_EXPECTED_FD, which can be
set along with IFLA_XDP_FD. If set, the kernel will check that the program
currently loaded on the interface matches the expected one, and fail the
operation if it does not. This corresponds to a 'cmpxchg' memory operation.

A new companion flag, XDP_FLAGS_EXPECT_FD, is also added to explicitly
request checking of the EXPECTED_FD attribute. This is needed for userspace
to discover whether the kernel supports the new attribute.

Signed-off-by: Toke Høiland-Jørgensen <redacted>

I didn't know we wanted to go ahead with this...

Well, I'm aware of the bpf_link discussion, obviously. Not sure what's
happening with that, though. So since this is a straight-forward
extension of the existing API, that doesn't carry a high implementation
cost, I figured I'd just go ahead with this. Doesn't mean we can't have
something similar in bpf_link as well, of course.

quoted

If we do please run this thru checkpatch, set .strict_start_type,

Will do.

quoted

and make the expected fd unsigned. A negative expected fd makes no
sense.

A negative expected_fd corresponds to setting the UPDATE_IF_NOEXIST
flag. I guess you could argue that since we have that flag, setting a
negative expected_fd is not strictly needed. However, I thought it was
weird to have a "this is what I expect" API that did not support
expressing "I expect no program to be attached".

For BPF syscall it seems the typical approach when optional FD is
needed is to have extra flag (e.g., BPF_F_REPLACE for cgroups) and if
it's not specified - enforce zero for that optional fd. That handles
backwards compatibility cases well as well.

Never did understand how that is supposed to square with 0 being a valid
fd number?

In BPF_F_REPLACE case (since it was used as an example in this thread)
it's all pretty clear:

* if the flag is set, use fd from attr.replace_bpf_fd that can be anything
  (incl. zero, since indeed it's valid fd) no problem with that;
* if flag is not set, ignore replace_bpf_fd completely.

It's descirbed in commit log in 7dd68b3279f1:

    ...

    BPF_F_REPLACE is introduced to make the user intent clear, since
    replace_bpf_fd alone can't be used for this (its default value, 0, is a
    valid fd). BPF_F_REPLACE also makes it possible to extend the API in the
    future (e.g. add BPF_F_BEFORE and BPF_F_AFTER if needed).

    ...

, i.e. flag presense is important, not the fd attribute being zero.

Hope it clarifies.


-- 
Andrey Ignatov