Hi,
On 11/8/2022 2:34 PM, Yonghong Song wrote:
>
>
> 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.
Will do in v2.
>
> 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.
I am trying to say that BPF_MAP_TYPE_PERF_EVENT_ARRAY doesn't have a problem
now, but if we add batch update to it, it will have the problem, because only
BPF_MAP_TYPE_PERF_EVENT_ARRAY uses the passed map_file in .map_fd_get_ptr(). I
will remove the words from 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>
> [...]
