On 11/19/19 2:09 PM, Yonghong Song wrote:
>
>
> On 11/19/19 2:00 PM, Alexei Starovoitov wrote:
>> On Tue, Nov 19, 2019 at 11:57:12AM -0800, Yonghong Song wrote:
>>> #define TNUM(_v, _m) (struct tnum){.value = _v, .mask = _m}
>>> -/* A completely unknown value */
>>> +/* completely unknown 32-bit and 64-bit values */
>>> +const struct tnum tnum_unknown32 = { .value = 0, .mask = 0xffffffffULL };
>>> const struct tnum tnum_unknown = { .value = 0, .mask = -1 };
>>>
>>> struct tnum tnum_const(u64 value)
>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>> index a344b08aef77..945827351758 100644
>>> --- a/kernel/bpf/verifier.c
>>> +++ b/kernel/bpf/verifier.c
>>> @@ -1024,6 +1024,15 @@ static void __mark_reg_unbounded(struct bpf_reg_state *reg)
>>> reg->umax_value = U64_MAX;
>>> }
>>>
>>> +/* Reset the min/max bounds of a sub register */
>>> +static void __mark_subreg_unbounded(struct bpf_reg_state *subreg)
>>> +{
>>> + subreg->smin_value = S32_MIN;
>>> + subreg->smax_value = S32_MAX;
>>> + subreg->umin_value = 0;
>>> + subreg->umax_value = U32_MAX;
>>> +}
>>
>> when int32 is returned the above feels correct, but I think it conflicts with
>> definition of tnum_unknown32, since it says that upper 32-bit should be zero.
>> The typical verifier action after processing alu32 insn:
>> if (BPF_CLASS(insn->code) != BPF_ALU64) {
>> /* 32-bit ALU ops are (32,32)->32 */
>> coerce_reg_to_size(dst_reg, 4);
>> }
>>
>> __reg_deduce_bounds(dst_reg);
>> __reg_bound_offset(dst_reg);
>>
>> And that is correct behavior for alu32, but here the helper is returning
>> 'int', so if the verifier says subreg->smin_value = S32_MIN;
>> it means that upper bits will be non-zero.
>> The helper can return (u64)-1 with all 64-bits being set to 1.
>> If next insn after w0 = call helper; is w0 += imm;
>> the verifier will do above coerce+deduce logic and clear upper bits.
>> That's correct, but without extra alu32 operation on w0 the state
>> of r0 is technically correct, but doesn't match r0->var_reg
>> which is tnum_unknown32.
>> I wonder whether it should be tnum_unknown instead with above
>> __mark_subreg_unbounded() ?
>
> tnum_unknown should work since subreg {smin,smax,umin,umax}_value
> all in 32-bit range. The mask (-1) should work as upper 32-bit unsigned
> value is always 0.
>
> Will make the change and send another revision.
An update for this bug.
Investigated further with Alexei and found actually we have
a LLVM bug where some LShift and Rshift is removed incorrectly
at +alu32 mode. The following LLVM patch
https://reviews.llvm.org/D70511/new/
has been merged which fixed LLVM issue.
There are some kernel changes needed to make test_progs passing, which
I will send out shortly.
This patch set can be abandoned.
>
>>
>>> +
>>> /* Mark a register as having a completely unknown (scalar) value. */
>>> static void __mark_reg_unknown(struct bpf_reg_state *reg)
>>> {
>>> @@ -1038,6 +1047,20 @@ static void __mark_reg_unknown(struct bpf_reg_state *reg)
>>> __mark_reg_unbounded(reg);
>>> }
>>>
>>> +/* Mark a sub register as having a completely unknown (scalar) value. */
>>> +static void __mark_subreg_unknown(struct bpf_reg_state *subreg)
>>> +{
>>> + /*
>>> + * Clear type, id, off, and union(map_ptr, range) and
>>> + * padding between 'type' and union
>>> + */
>>> + memset(subreg, 0, offsetof(struct bpf_reg_state, var_off));
>>> + subreg->type = SCALAR_VALUE;
>>> + subreg->var_off = tnum_unknown32;
>>> + subreg->frameno = 0;
>>> + __mark_subreg_unbounded(subreg);
>>> +}
>>
