On Wed, Jul 22, 2020 at 12:18 PM Maciej Fijalkowski
[off-list ref] wrote:

On Tue, Jul 21, 2020 at 11:45:56PM -0700, Andrii Nakryiko wrote:

quoted

Further refactor XDP attachment code. dev_change_xdp_fd() is split into two
parts: getting bpf_progs from FDs and attachment logic, working with
bpf_progs. This makes attachment  logic a bit more straightforward and
prepares code for bpf_xdp_link inclusion, which will share the common logic.

Signed-off-by: Andrii Nakryiko <redacted>
---
 net/core/dev.c | 165 +++++++++++++++++++++++++++----------------------
 1 file changed, 91 insertions(+), 74 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 7e753e248cef..abf573b2dcf4 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -8815,111 +8815,128 @@ static void dev_xdp_uninstall(struct net_device *dev)
      }
 }

-/**
- *   dev_change_xdp_fd - set or clear a bpf program for a device rx path
- *   @dev: device
- *   @extack: netlink extended ack
- *   @fd: new program fd or negative value to clear
- *   @expected_fd: old program fd that userspace expects to replace or clear
- *   @flags: xdp-related flags
- *
- *   Set or clear a bpf program for a device
- */
-int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
-                   int fd, int expected_fd, u32 flags)
+static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack,
+                       struct bpf_prog *new_prog, struct bpf_prog *old_prog,
+                       u32 flags)
 {
-     const struct net_device_ops *ops = dev->netdev_ops;
-     enum bpf_xdp_mode mode = dev_xdp_mode(flags);
-     bool offload = mode == XDP_MODE_HW;
-     u32 prog_id, expected_id = 0;
-     struct bpf_prog *prog;
+     struct bpf_prog *cur_prog;
+     enum bpf_xdp_mode mode;
      bpf_op_t bpf_op;
      int err;

      ASSERT_RTNL();

couldn't we rely on caller's rtnl assertion? dev_change_xdp_fd() already
has one.

dev_xdp_attach() is also used from the bpf_link attaching function
(dev_xdp_attach_link() in the later patch). I can remove ASSERT_RTNL()
from dev_change_xdp_fd(), though, it doesn't have to do that check, if
dev_xdp_attach() does it already.

[...]

quoted

-
-             if (prog->expected_attach_type == BPF_XDP_CPUMAP) {
-                     NL_SET_ERR_MSG(extack,
-                                    "BPF_XDP_CPUMAP programs can not be attached to a device");
-                     bpf_prog_put(prog);
+             if (new_prog->expected_attach_type == BPF_XDP_CPUMAP) {
+                     NL_SET_ERR_MSG(extack, "BPF_XDP_CPUMAP programs can not be attached to a device");

bpf_prog_put() missing?

Nope, program putting on error is handled outside the
dev_xdp_attach(), either by bpf() LINK_CREATE handling logic or by
dev_change_xdp_fd().

quoted

                      return -EINVAL;
              }
+     }

[...]