Re: [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs attached to sockmap

["zhudi (E)" <zhudi2@huawei.com>]

> On Thu, Nov 04, 2021 at 09:07:44AM +0800, Di Zhu wrote:
> > +int sock_map_bpf_prog_query(const union bpf_attr *attr,
> > +			    union bpf_attr __user *uattr)
> > +{
> > +	__u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
> > +	u32 prog_cnt = 0, flags = 0, ufd = attr->target_fd;
> > +	struct bpf_prog **pprog;
> > +	struct bpf_prog *prog;
> > +	struct bpf_map *map;
> > +	struct fd f;
> > +	u32 id = 0;
> > +	int ret;
> > +
> > +	if (attr->query.query_flags)
> > +		return -EINVAL;
> > +
> > +	f = fdget(ufd);
> > +	map = __bpf_map_get(f);
> > +	if (IS_ERR(map))
> > +		return PTR_ERR(map);
> > +
> > +	rcu_read_lock();
> > +
> > +	ret = sock_map_prog_lookup(map, &pprog, attr->query.attach_type);
> > +	if (ret)
> > +		goto end;
> > +
> > +	prog = *pprog;
> > +	prog_cnt = (!prog) ? 0 : 1;
> > +
> > +	if (!attr->query.prog_cnt || !prog_ids || !prog_cnt)
> > +		goto end;
> 

> This sanity check (except prog_cnt) can be moved before RCU read lock?

I think we should call sock_map_prog_lookup() in any case. Because we can
just return query results(such as -EOPNOTSUPP) which may not care about
the prog_ids.

So this sanity check should right behind this call and must be in rcu critical zone.

> > +
> > +	id = prog->aux->id;
> > +	if (id == 0)
> > +		prog_cnt = 0;
> 
> The id seems generic, so why not handle it in bpf_prog_query() for all progs?

The prog id is a generic, but different progs have different organizational forms, 
so they can only be handled differently at present...

> > +
> > +end:
> > +	rcu_read_unlock();
> > +
> > +	if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags)) ||
> 
> 'flags' is always 0 here, right? So this is not needed as uattr has been already
> cleared in __sys_bpf().

 I recheck the code, it seems that __sys_bpf() does not do this clear things.

 
> Thanks.