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

Message-ID: <20190123011057.bgkpt2fykbv3dm25@kafai-mbp.dhcp.thefacebook.com>
Date:   Wed, 23 Jan 2019 01:10:58 +0000
From:   Martin Lau <kafai@...com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.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, Jan 22, 2019 at 04:12:51PM -0800, Jakub Kicinski wrote:
> > > +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.
Thanks for the explanation.  Make sense after another thought.
It would be very helpful to add another comment here.

In general, I feel the bpf_adj_line_after_remove() is quite
tricky to read....could be me slow only.