Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 5 Nov 2021 12:51:55 -0700
From:   Cong Wang <xiyou.wangcong@gmail.com>
To:     Di Zhu <zhudi2@huawei.com>
Cc:     davem@davemloft.net, ast@kernel.org, daniel@iogearbox.net,
        andrii@kernel.org, kafai@fb.com, songliubraving@fb.com, yhs@fb.com,
        john.fastabend@gmail.com, kpsingh@kernel.org, jakub@cloudflare.com,
        bpf@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs
 attached to sockmap
Message-ID: <YYWLW5IJhmIa2aVX@unknown>
References: <20211104010745.1177032-1-zhudi2@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211104010745.1177032-1-zhudi2@huawei.com>
Precedence: bulk

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?

> +
> +	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?

> +
> +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().


Thanks.