netdev - Re: [PATCH bpf-next v5 03/12] bpf: verifier: remove dead code

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190122161251.799e1ebf@cakuba.netronome.com>
Date:   Tue, 22 Jan 2019 16:12:51 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Martin Lau <kafai@...com>
Cc:     "alexei.starovoitov@...il.com" <alexei.starovoitov@...il.com>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "oss-drivers@...ronome.com" <oss-drivers@...ronome.com>,
        Yonghong Song <yhs@...com>
Subject: Re: [PATCH bpf-next v5 03/12] bpf: verifier: remove dead code

On Tue, 22 Jan 2019 23:46:34 +0000, Martin Lau wrote:
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 71949c163b7a..fa8011409c51 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -6431,6 +6431,144 @@ static struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 of
> >  	return new_prog;
> >  }
> >  
> > +static int adjust_subprog_starts_after_remove(struct bpf_verifier_env *env,
> > +					      u32 off, u32 cnt)
> > +{
> > +	int i, j;
> > +
> > +	/* find first prog starting at or after off (first to remove) */
> > +	for (i = 0; i < env->subprog_cnt; i++)
> > +		if (env->subprog_info[i].start >= off)
> > +			break;
> > +	/* find first prog starting at or after off + cnt (first to stay) */
> > +	for (j = i; j < env->subprog_cnt; j++)
> > +		if (env->subprog_info[j].start >= off + cnt)
> > +			break;
> > +	/* if j doesn't start exactly at off + cnt, we are just removing
> > +	 * the front of previous prog
> > +	 */
> > +	if (env->subprog_info[j].start != off + cnt)
> > +		j--;
> > +
> > +	if (j > i) {
> > +		struct bpf_prog_aux *aux = env->prog->aux;
> > +		int move;
> > +
> > +		/* move fake 'exit' subprog as well */
> > +		move = env->subprog_cnt + 1 - j;
> > +
> > +		memmove(env->subprog_info + i,
> > +			env->subprog_info + j,
> > +			sizeof(*env->subprog_info) * move);
> > +		env->subprog_cnt -= j - i;
> > +
> > +		/* remove func_info */  
> 
> It would be helpful to add a comment here to explain
> that func_info->insn_off is set later in adjust_btf_func(),
> so no need to adjust it here.

Will do!

> > +		if (aux->func_info) {
> > +			move = aux->func_info_cnt - j;
> > +
> > +			memmove(aux->func_info + i,
> > +				aux->func_info + j,
> > +				sizeof(*aux->func_info) * move);
> > +			aux->func_info_cnt -= j - i;
> > +		}
> > +	} else {
> > +		/* convert i from "first prog to remove" to "first to adjust" */
> > +		if (env->subprog_info[i].start == off)
> > +			i++;
> > +	}
> > +
> > +	/* update fake 'exit' subprog as well */
> > +	for (; i <= env->subprog_cnt; i++)
> > +		env->subprog_info[i].start -= cnt;
> > +
> > +	return 0;
> > +}
> > +
> > +static int bpf_adj_linfo_after_remove(struct bpf_verifier_env *env, u32 off,
> > +				      u32 cnt)
> > +{
> > +	struct bpf_prog *prog = env->prog;
> > +	u32 i, l_off, l_cnt, nr_linfo;
> > +	struct bpf_line_info *linfo;
> > +
> > +	nr_linfo = prog->aux->nr_linfo;
> > +	if (!nr_linfo)
> > +		return 0;
> > +
> > +	linfo = prog->aux->linfo;
> > +
> > +	/* find first line info to remove, count lines to be removed */
> > +	for (i = 0; i < nr_linfo; i++)
> > +		if (linfo[i].insn_off >= off)
> > +			break;
> > +
> > +	l_off = i;
> > +	l_cnt = 0;
> > +	for (; i < nr_linfo; i++)
> > +		if (linfo[i].insn_off < off + cnt)
> > +			l_cnt++;
> > +		else
> > +			break;
> > +
> > +	/* First live insn doesn't match first live linfo, it needs to "inherit"
> > +	 * last removed linfo.  prog is already modified, so prog->len == off
> > +	 * means no live instructions after.
> > +	 */
> > +	if (prog->len != off && l_cnt &&
> > +	    (i == nr_linfo || linfo[i].insn_off != off + cnt)) {
> > +		l_cnt--;
> > +		linfo[--i].insn_off = off + cnt;
> > +	}
> > +
> > +	/* remove the line info which refers to the removed instructions */
> > +	if (l_cnt) {
> > +		memmove(linfo + l_off, linfo + i,
> > +			sizeof(*linfo) * (nr_linfo - i));
> > +
> > +		prog->aux->nr_linfo -= l_cnt;
> > +		nr_linfo = prog->aux->nr_linfo;
> > +	}
> > +
> > +	/* pull all linfo[i].insn_off >= off + cnt in by cnt */
> > +	for (i = l_off; i < nr_linfo; i++)
> > +		linfo[i].insn_off -= cnt;
> > +
> > +	/* fix up all subprogs (incl. 'exit') which start >= off */
> > +	for (i = 0; i <= env->subprog_cnt; i++)
> > +		if (env->subprog_info[i].linfo_idx > l_off) {
> > +			if (env->subprog_info[i].linfo_idx >= l_off + l_cnt)
> > +				env->subprog_info[i].linfo_idx -= l_cnt;
> > +			else
> > +				env->subprog_info[i].linfo_idx = l_off;  
> 
> For l_off < linfo_idx < l_off + lcnt, had those subprog_info already been
> removed in adjust_subprog_starts_after_remove()?

If we remove tail of one program and start of another this will set the
linfo_idx to the new first instruction's linfo_idx.

> > +		}
> > +
> > +	return 0;
> > +}