On Mon, Mar 23, 2020 at 4:25 AM Toke Høiland-Jørgensen [off-list ref] wrote:

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?

You mean a tiny chance that given invalid userspace program behavior
(setting valid FD 0 without specifying BPF_F_REPLACE or not setting
FD, but FD=0 being a valid program FD) it might succeed accidentally?
Sure it's theoretically possible, but highly unlikely and in any case
it's an invalid userspace behavior. So I guess it was deemed
acceptable for the sake of backwards compatibility?

-Toke