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 ...
> > 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__.
Thanks,
Daniel
