Re: [PATCH v4 bpf-next 02/15] bpf: mark lo32 writes that should be zero extended into hi32

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Jakub Kicinski <jakub.kicinski@netronome.com>
Cc: Jiong Wang <jiong.wang@netronome.com>,
	daniel@iogearbox.net, bpf@vger.kernel.org,
	netdev@vger.kernel.org, oss-drivers@netronome.com
Subject: Re: [PATCH v4 bpf-next 02/15] bpf: mark lo32 writes that should be zero extended into hi32
Date: Fri, 19 Apr 2019 14:14:05 -0700	[thread overview]
Message-ID: <20190419211403.6iovh26bu6cg2x36@ast-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <20190419134051.71eeea08@cakuba.netronome.com>

On Fri, Apr 19, 2019 at 01:40:51PM -0700, Jakub Kicinski wrote:
> On Thu, 18 Apr 2019 16:57:50 -0700, Alexei Starovoitov wrote:
> > > @@ -6371,8 +6406,10 @@ static int propagate_liveness(struct bpf_verifier_env *env,
> > >  		for (i = frame < vstate->curframe ? BPF_REG_6 : 0; i < BPF_REG_FP; i++) {
> > >  			err = propagate_liveness_reg(env, &state_reg[i],
> > >  						     &parent_reg[i]);
> > > -			if (err)
> > > +			if (err < 0)
> > >  				return err;
> > > +			if (err & REG_LIVE_READ64)
> > > +				mark_insn_zext(env, &parent_reg[i]);  
> > 
> > I'm not quite following why it's parent_reg here instead of state_reg.
> 
> Perhaps we should rename the parameters to something else than parent
> here?  I always have to do some mental gymnastics looking at this code..
> "explored" and "current"?
> 
> The current state is parent, the "next" state that pruned the search
> is "state".  So we check if the reads under state X need 64bit, if so
> have to propagate back to writes on current (which is called parent
> here, even though it won't become state's parent, ugh.)

agree :)

> > If I understood the code the liveness can have all three states:
> > REG_LIVE_READ64 | REG_LIVE_READ32
> > REG_LIVE_READ64
> > REG_LIVE_READ32
> > whereas 2 is a superset of 3, so 1 should never be seen.
> > 
> > If so, why in propagate_liveness we have this dance:
> > +       u8 parent_bits = parent_reg->live & REG_LIVE_READ;
> > +       u8 bits = reg->live & REG_LIVE_READ;
> > +       u8 bits_diff = parent_bits ^ bits;
> > +       u8 bits_prop = bits_diff & bits;
> >         int err;
> > 
> > -       if (parent_reg->live & REG_LIVE_READ || !(reg->live & REG_LIVE_READ))
> > +       /* No diff bit comes from "reg". */
> > +       if (!bits_prop)
> > 
> > I'm struggling to see through all 3 combinations in respect to above diff.
> > Shouldn't propagation of REG_LIVE_READ64 remove REG_LIVE_READ32 bit
> > and clear subreg_def during mark_reg_read() instead of
> > once in propagate_liveness() ?
> 
> This reminds me, I'm not entirely clear on the need to propagate the
> zext through stack slots...  Pointers are guaranteed to be 64bit, we
> don't save parentage on scalars (AFAICT),

scalars have parentage chain too.
we don't track them precisely when they're spilled to stack.
That actually caused an issue recently when valid program was rejected,
so we might add a feature to track full contents of scalars in the stack.

> why not pass REG_LIVE_READ
> or READ64 to mark_reg_read() from stack_read?

can we agree on only two states first ? ;)