From: sdf@google.com
To: Hou Tao <houtao@huaweicloud.com>
Cc: bpf@vger.kernel.org, Yonghong Song <yhs@fb.com>,
Martin KaFai Lau <martin.lau@linux.dev>,
Andrii Nakryiko <andrii@kernel.org>, Song Liu <song@kernel.org>,
Hao Luo <haoluo@google.com>, Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
KP Singh <kpsingh@kernel.org>, Jiri Olsa <jolsa@kernel.org>,
John Fastabend <john.fastabend@gmail.com>,
Xu Kuohai <xukuohai@huawei.com>,
houtao1@huawei.com
Subject: Re: [PATCH bpf-next v2] bpf: Pass map file to .map_update_batch directly
Date: Fri, 11 Nov 2022 09:43:18 -0800 [thread overview]
Message-ID: <Y26JtknJKjnD+dsu@google.com> (raw)
In-Reply-To: <20221111080757.2224969-1-houtao@huaweicloud.com>
On 11/11, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
> Currently bpf_map_do_batch() first invokes fdget(batch.map_fd) to get
> the target map file, then it invokes generic_map_update_batch() to do
> batch update. generic_map_update_batch() will get the target map file
> by using fdget(batch.map_fd) again and pass it to
> bpf_map_update_value().
> The problem is map file returned by the second fdget() may be NULL or a
> totally different file compared by map file in bpf_map_do_batch(). The
> reason is that the first fdget() only guarantees the liveness of struct
> file instead of file descriptor and the file description may be released
> by concurrent close() through pick_file().
> It doesn't incur any problem as for now, because maps with batch update
> support don't use map file in .map_fd_get_ptr() ops. But it is better to
> fix the access of a potentially invalid map file.
> using __bpf_map_get() again in generic_map_update_batch() can not fix
> the problem, because batch.map_fd may be closed and reopened, and the
> returned map file may be different with map file got in
> bpf_map_do_batch(), so just passing the map file directly to
> .map_update_batch() in bpf_map_do_batch().
> Signed-off-by: Hou Tao <houtao1@huawei.com>
Acked-by: Stanislav Fomichev <sdf@google.com>
> ---
> v2:
> * rewrite the commit message to explain the problem and the reasoning.
> v1:
> https://lore.kernel.org/bpf/20221107075537.1445644-1-houtao@huaweicloud.com
> include/linux/bpf.h | 5 +++--
> kernel/bpf/syscall.c | 31 ++++++++++++++++++-------------
> 2 files changed, 21 insertions(+), 15 deletions(-)
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 798aec816970..20cfe88ee6df 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -85,7 +85,8 @@ struct bpf_map_ops {
> int (*map_lookup_and_delete_batch)(struct bpf_map *map,
> const union bpf_attr *attr,
> union bpf_attr __user *uattr);
> - int (*map_update_batch)(struct bpf_map *map, const union bpf_attr *attr,
> + int (*map_update_batch)(struct bpf_map *map, struct file *map_file,
> + const union bpf_attr *attr,
> union bpf_attr __user *uattr);
> int (*map_delete_batch)(struct bpf_map *map, const union bpf_attr *attr,
> union bpf_attr __user *uattr);
> @@ -1776,7 +1777,7 @@ void bpf_map_init_from_attr(struct bpf_map *map,
> union bpf_attr *attr);
> int generic_map_lookup_batch(struct bpf_map *map,
> const union bpf_attr *attr,
> union bpf_attr __user *uattr);
> -int generic_map_update_batch(struct bpf_map *map,
> +int generic_map_update_batch(struct bpf_map *map, struct file *map_file,
> const union bpf_attr *attr,
> union bpf_attr __user *uattr);
> int generic_map_delete_batch(struct bpf_map *map,
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 85532d301124..cb8a87277bf8 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -175,8 +175,8 @@ static void maybe_wait_bpf_programs(struct bpf_map
> *map)
> synchronize_rcu();
> }
> -static int bpf_map_update_value(struct bpf_map *map, struct fd f, void
> *key,
> - void *value, __u64 flags)
> +static int bpf_map_update_value(struct bpf_map *map, struct file
> *map_file,
> + void *key, void *value, __u64 flags)
> {
> int err;
> @@ -190,7 +190,7 @@ static int bpf_map_update_value(struct bpf_map *map,
> struct fd f, void *key,
> map->map_type == BPF_MAP_TYPE_SOCKMAP) {
> return sock_map_update_elem_sys(map, key, value, flags);
> } else if (IS_FD_PROG_ARRAY(map)) {
> - return bpf_fd_array_map_update_elem(map, f.file, key, value,
> + return bpf_fd_array_map_update_elem(map, map_file, key, value,
> flags);
> }
> @@ -205,12 +205,12 @@ static int bpf_map_update_value(struct bpf_map
> *map, struct fd f, void *key,
> flags);
> } else if (IS_FD_ARRAY(map)) {
> rcu_read_lock();
> - err = bpf_fd_array_map_update_elem(map, f.file, key, value,
> + err = bpf_fd_array_map_update_elem(map, map_file, key, value,
> flags);
> rcu_read_unlock();
> } else if (map->map_type == BPF_MAP_TYPE_HASH_OF_MAPS) {
> rcu_read_lock();
> - err = bpf_fd_htab_map_update_elem(map, f.file, key, value,
> + err = bpf_fd_htab_map_update_elem(map, map_file, key, value,
> flags);
> rcu_read_unlock();
> } else if (map->map_type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY) {
> @@ -1390,7 +1390,7 @@ static int map_update_elem(union bpf_attr *attr,
> bpfptr_t uattr)
> goto free_key;
> }
> - err = bpf_map_update_value(map, f, key, value, attr->flags);
> + err = bpf_map_update_value(map, f.file, key, value, attr->flags);
> kvfree(value);
> free_key:
> @@ -1576,16 +1576,14 @@ int generic_map_delete_batch(struct bpf_map *map,
> return err;
> }
> -int generic_map_update_batch(struct bpf_map *map,
> +int generic_map_update_batch(struct bpf_map *map, struct file *map_file,
> const union bpf_attr *attr,
> union bpf_attr __user *uattr)
> {
> void __user *values = u64_to_user_ptr(attr->batch.values);
> void __user *keys = u64_to_user_ptr(attr->batch.keys);
> u32 value_size, cp, max_count;
> - int ufd = attr->batch.map_fd;
> void *key, *value;
> - struct fd f;
> int err = 0;
> if (attr->batch.elem_flags & ~BPF_F_LOCK)
> @@ -1612,7 +1610,6 @@ int generic_map_update_batch(struct bpf_map *map,
> return -ENOMEM;
> }
> - f = fdget(ufd); /* bpf_map_do_batch() guarantees ufd is valid */
> for (cp = 0; cp < max_count; cp++) {
> err = -EFAULT;
> if (copy_from_user(key, keys + cp * map->key_size,
> @@ -1620,7 +1617,7 @@ int generic_map_update_batch(struct bpf_map *map,
> copy_from_user(value, values + cp * value_size, value_size))
> break;
> - err = bpf_map_update_value(map, f, key, value,
> + err = bpf_map_update_value(map, map_file, key, value,
> attr->batch.elem_flags);
> if (err)
> @@ -1633,7 +1630,6 @@ int generic_map_update_batch(struct bpf_map *map,
> kvfree(value);
> kvfree(key);
> - fdput(f);
> return err;
> }
> @@ -4435,6 +4431,15 @@ static int bpf_task_fd_query(const union bpf_attr
> *attr,
> err = fn(map, attr, uattr); \
> } while (0)
[..]
> +#define BPF_DO_BATCH_WITH_FILE(fn) \
> + do { \
> + if (!fn) { \
> + err = -ENOTSUPP; \
> + goto err_put; \
> + } \
> + err = fn(map, f.file, attr, uattr); \
> + } while (0)
> +
nit: probably not worth defining this for a single user? but not sure
it matters..
> static int bpf_map_do_batch(const union bpf_attr *attr,
> union bpf_attr __user *uattr,
> int cmd)
> @@ -4470,7 +4475,7 @@ static int bpf_map_do_batch(const union bpf_attr
> *attr,
> else if (cmd == BPF_MAP_LOOKUP_AND_DELETE_BATCH)
> BPF_DO_BATCH(map->ops->map_lookup_and_delete_batch);
> else if (cmd == BPF_MAP_UPDATE_BATCH)
> - BPF_DO_BATCH(map->ops->map_update_batch);
> + BPF_DO_BATCH_WITH_FILE(map->ops->map_update_batch);
> else
> BPF_DO_BATCH(map->ops->map_delete_batch);
> err_put:
> --
> 2.29.2
next prev parent reply other threads:[~2022-11-11 17:43 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-11 8:07 [PATCH bpf-next v2] bpf: Pass map file to .map_update_batch directly Hou Tao
2022-11-11 17:43 ` sdf [this message]
2022-11-14 17:49 ` Daniel Bokmann
2022-11-15 11:18 ` Hou Tao
2022-11-11 23:02 ` Yonghong Song
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y26JtknJKjnD+dsu@google.com \
--to=sdf@google.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=haoluo@google.com \
--cc=houtao1@huawei.com \
--cc=houtao@huaweicloud.com \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kpsingh@kernel.org \
--cc=martin.lau@linux.dev \
--cc=song@kernel.org \
--cc=xukuohai@huawei.com \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox