Re: [PATCH bpf-next v2] bpf: Pass map file to .map_update_batch directly

[Daniel Bokmann <darkstar@linux.home>]

On Fri, Nov 11, 2022 at 09:43:18AM -0800, sdf@google.com wrote:
> 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.

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@huawei.com>
> 
> Acked-by: Stanislav Fomichev <sdf@google.com>

> [..]
> 
> > +#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