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: <70F0E85E-B8B3-49AB-ACA7-906A9D529240@netronome.com>
Date:   Thu, 11 Apr 2019 18:22:16 +0100
From:   Jiong Wang <jiong.wang@...ronome.com>
To:     Jakub Kicinski <jakub.kicinski@...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 11 Apr 2019, at 17:44, Jakub Kicinski <jakub.kicinski@...ronome.com> wrote:
> 
> 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..

Ack, indeed, !bits_prop is always true if !bits_diff is true, will remove the
redundant check.

Thanks,
Regards,
Jiong