netdev - Re: [oss-drivers] Re: [PATCH/RFC v2 bpf-next 05/19] bpf: split read liveness into REG_LIVE_READ64 and REG_LIVE

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: <20190411094456.1fabc52d@cakuba.netronome.com>
Date:   Thu, 11 Apr 2019 09:44:56 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Jiong Wang <jiong.wang@...ronome.com>
Cc:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        daniel@...earbox.net, bpf@...r.kernel.org, netdev@...r.kernel.org,
        oss-drivers@...ronome.com
Subject: Re: [oss-drivers] Re: [PATCH/RFC v2 bpf-next 05/19] bpf: split read
 liveness into REG_LIVE_READ64 and REG_LIVE_READ32

On Thu, 11 Apr 2019 07:13:03 +0100, Jiong Wang wrote:
> >> @@ -1150,17 +1150,17 @@ static int mark_reg_read(struct bpf_verifier_env *env,
> >> 				parent->var_off.value, parent->off);
> >> 			return -EFAULT;
> >> 		}
> >> -		if (parent->live & REG_LIVE_READ)
> >> +		if ((parent->live & REG_LIVE_READ) == flags)
> >> 			/* The parentage chain never changes and
> >> -			 * this parent was already marked as LIVE_READ.
> >> +			 * this parent was already marked with all read bits.  
> > 
> > Do we have to propagate all read bits?  Read64 is strictly stronger
> > than read32, as long as read64 is set on the parent we should be good?  
> 
> We should be good, but I doubt there is value to differentiate on this in this
> kind of HOT function.

The entire if clause is an optimization.  I'm saying you can maintain it
as more aggressive.

> >> @@ -6196,12 +6286,19 @@ static int propagate_liveness_reg(struct bpf_verifier_env *env,
> >> 				  struct bpf_reg_state *reg,
> >> 				  struct bpf_reg_state *parent_reg)
> >> {
> >> +	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))
> >> +	/* "reg" and "parent_reg" has the same read bits, or the bit doesn't
> >> +	 * belong to "reg".
> >> +	 */
> >> +	if (!bits_diff || !bits_prop)  
> > 
> > bits_prop is a subset of bits_diff, no?  !bits_prop is always true
> > if !bits_diff is true, no need to check both.  
> 
> Bits_prop is a subset of bits_diff WHEN it comes from “reg", we don’t want to
> do the propagation when the diff comes from “parent_reg”, so, we need to check
> both.

Not sure what you're saying, in this patch:

	u8 bits_prop = bits_diff & bits;

Maybe you're talking about some patch down the line..