On 11/7/22 4:53 PM, Hou Tao wrote:
Hi,
On 11/8/2022 8:08 AM, Yonghong Song wrote:
On 11/6/22 11:55 PM, Hou Tao wrote:
From: Hou Tao <houtao1@xxxxxxxxxx>
Currently generic_map_update_batch() will get map file from
attr->batch.map_fd and pass it to bpf_map_update_value(). The problem is
map_fd may have been closed or reopened as a different file type and
generic_map_update_batch() doesn't check the validity of map_fd.
It doesn't incur any problem as for now, because only
BPF_MAP_TYPE_PERF_EVENT_ARRAY uses the passed map file and it doesn't
support batch update operation. But it is better to fix the potential
use of an invalid map file.
I think we don't have problem here. The reason is in bpf_map_do_batch()
we have
f = fdget(ufd);
...
BPF_DO_BATCH(map->ops->map_update_batch);
fdput(f)
So the original ufd is still valid during map->ops->map_update_batch
which eventually may call generic_map_update_batch() which tries to
do fdget(ufd) again.
The previous fdget() only guarantees the liveness of struct file. If the map fd
is closed by another thread concurrently, the fd will released by pick_file() as
show below:
static struct file *pick_file(struct files_struct *files, unsigned fd)
{
struct fdtable *fdt = files_fdtable(files);
struct file *file;
file = fdt->fd[fd];
if (file) {
rcu_assign_pointer(fdt->fd[fd], NULL);
__put_unused_fd(files, fd);
}
return file;
}
So the second fdget(udf) may return a NULL file or a different file.
Okay. Thanks for explanation. It would be great if you can describe
the above reasoning clearly in the commit message since this is
where the bug exists.
Not sure why BPF_MAP_TYPE_PERF_EVENT_ARRAY matters here. Or you just
show BPF_MAP_TYPE_PERF_EVENT_ARRAY does not have a problem. If this
is the case, there is no need to mention it in the commit message.
Did I miss anything here?
Checking the validity of map file returned from fdget() in
generic_map_update_batch() can not fix the problem, because the returned
map file may be different with map file got in bpf_map_do_batch() due to
the reopening of fd, so just passing the map file directly to
.map_update_batch() in bpf_map_do_batch().
Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
[...]
