linux-kernel - Re: [PATCH bpf-next v5 1/2] bpf: support BPF_PROG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:   Mon, 8 Nov 2021 02:13:19 +0000
From:   "zhudi (E)" <zhudi2@...wei.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "andrii@...nel.org" <andrii@...nel.org>,
        "kafai@...com" <kafai@...com>,
        "songliubraving@...com" <songliubraving@...com>,
        "yhs@...com" <yhs@...com>,
        "john.fastabend@...il.com" <john.fastabend@...il.com>,
        "kpsingh@...nel.org" <kpsingh@...nel.org>,
        "jakub@...udflare.com" <jakub@...udflare.com>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs
 attached to sockmap

> 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.