libxdp: always clone program fd before taking ownership of it

Coverity pointed out that we introduced use after free errors when
converting xdp_program__from_fd() to call xdp_program__close() instead of
just freeing the xdp_program object. However, we do need to call the
destructor or we won't free up all resources. At the same time, we don't
want xdp_program__from_fd() to take ownership of the fd, since that
function is also public API.

To fix this, change xdp_program__fill_from_fd() so it always duplicates the
fd before filling in the object, taking ownership of the duplicate instead
of the original fd. We already did this in a couple of call sites, which we
can then get rid of, leading to an overall simplification.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

lib/libxdp/libxdp.c

@@ -1113,12 +1113,20 @@ static int xdp_program__fill_from_fd(struct xdp_program *xdp_prog, int fd)
 	struct bpf_prog_info info = {};
 	__u32 len = sizeof(info);
 	struct btf *btf = NULL;
-	int err = 0;
+	int err = 0, prog_fd;
 
 	if (!xdp_prog)
 		return -EINVAL;
 
-	err = bpf_obj_get_info_by_fd(fd, &info, &len);
+	/* Duplicate the descriptor, as we take ownership of the fd below */
+	prog_fd = fcntl(fd, F_DUPFD_CLOEXEC, MIN_FD);
+	if (prog_fd < 0) {
+		err = -errno;
+		pr_debug("Error on fcntl: %s", strerror(-err));
+		return err;
+	}
+
+	err = bpf_obj_get_info_by_fd(prog_fd, &info, &len);
 	if (err) {
 		err = -errno;
 		pr_warn("couldn't get program info: %s", strerror(-err));
@@ -1145,11 +1153,12 @@ static int xdp_program__fill_from_fd(struct xdp_program *xdp_prog, int fd)
 
 	memcpy(xdp_prog->prog_tag, info.tag, BPF_TAG_SIZE);
 	xdp_prog->load_time = info.load_time;
-	xdp_prog->prog_fd = fd;
+	xdp_prog->prog_fd = prog_fd;
 	xdp_prog->prog_id = info.id;
 
 	return 0;
 err:
+	close(prog_fd);
 	btf__free(btf);
 	return err;
 }
@@ -1269,7 +1278,7 @@ int xdp_program__pin(struct xdp_program *prog, const char *pin_path)
 
 static int xdp_program__load(struct xdp_program *prog)
 {
-	int err;
+	int err, prog_fd;
 
 	if (!prog)
 		return -EINVAL;
@@ -1284,42 +1293,25 @@ static int xdp_program__load(struct xdp_program *prog)
 	if (err)
 		return err;
 
+	prog_fd = bpf_program__fd(prog->bpf_prog);
 	pr_debug("Loaded XDP program %s, got fd %d\n",
-		 xdp_program__name(prog), bpf_program__fd(prog->bpf_prog));
-
-	/* Duplicate the descriptor, as xdp_program__fill_from_fd takes ownership */
-	int prog_fd = fcntl(bpf_program__fd(prog->bpf_prog), F_DUPFD_CLOEXEC, MIN_FD);
-	if (prog_fd < 0) {
-		err = -errno;
-		pr_debug("Error on fcntl: %s", strerror(-err));
-		return err;
-	}
+		 xdp_program__name(prog), prog_fd);
 
+	/* xdp_program__fill_from_fd() clones the fd and takes ownership of the clone */
 	return xdp_program__fill_from_fd(prog, prog_fd);
 }
 
-struct xdp_program *xdp_program__clone(struct xdp_program *prog)
+struct xdp_program *xdp_program__clone(struct xdp_program *prog, unsigned int flags)
 {
-	struct xdp_program *new_prog;
-	int new_fd, err;
-
-	/* Clone a loaded program struct by duplicating the fd and creating a
-	 * new structure from the kernel state.
-	 */
 	if (!prog || prog->prog_fd < 0)
 		return ERR_PTR(-EINVAL);
 
-	new_fd = fcntl(prog->prog_fd, F_DUPFD_CLOEXEC, MIN_FD);
-	if (new_fd < 0) {
-		err = -errno;
-		pr_debug("Error on fcntl: %s\n", strerror(-err));
-		return ERR_PTR(err);
-	}
-
-	new_prog = xdp_program__from_fd(new_fd);
-	if (IS_ERR(new_prog))
-		close(new_fd);
-	return new_prog;
+	/* Clone a loaded program struct by creating a new object from the
+	   program fd; xdp_program__fill_from_fd() already duplicates the fd
+	   before filling in the object, so this creates a completely
+	   independent xdp_program object.
+	*/
+	return xdp_program__from_fd(prog->prog_fd);
 }
 
 static int xdp_program__attach_single(struct xdp_program *prog, int ifindex,