Hi,
On 11/15/2022 1:49 AM, Daniel Bokmann wrote:
> On Fri, Nov 11, 2022 at 09:43:18AM -0800, sdf@xxxxxxxxxx wrote:
>> On 11/11, Hou Tao wrote:
>>> From: Hou Tao <houtao1@xxxxxxxxxx>
>>> 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.
> Right, that's mainly for the perf RB map ...
Yes. BPF_MAP_TYPE_PERF_EVENT_ARRAY will use the passed map file, but it doesn't
support batch update.
>
>>> 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@xxxxxxxxxx>
>> Acked-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
>> [..]
>>
>>> +#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..
> Yeah, just the BPF_DO_BATCH could be used but extended via __VA_ARGS__.
Good idea. Will do in v3.
>
> Thanks,
> Daniel
