On Tue, Jul 23, 2024 at 12:07 AM Shung-Hsi Yu [off-list ref] wrote:

On Tue, Jul 23, 2024 at 02:36:18PM GMT, Shung-Hsi Yu wrote:
[...]

quoted

quoted

+1
Pls document the logic in the code.
commit log is good, but good chunk of it probably should be copied
as a comment.

I've applied the rest of the patches and removed 'test 3' selftest.
Pls respin this patch and a test.
More than one test would be nice too.

Ack. Will send send another series that:

1. update current patch
  - add code comment explanation how signed ranges are deduced in
    scalar*_min_max_and()
  - revert 229d6db14942 "selftests/bpf: Workaround strict bpf_lsm return
    value check."
2. reintroduce Xu Kuohai's "test 3" into verifier_lsm.c
3. add a few tests for BPF_AND's signed range deduction
   - should it be added to verifier_bounds*.c or verifier_and.c?

     I think former, because if we later add signed range deduction for
     BPF_OR as well...

I was curious whether there would be imminent need for signed range
deduction for BPF_OR, though looks like there is _not_.

Looking at DAGCombiner::SimplifySelectCC() it does not do the
bitwise-OR variant of what we've encountered[1,2], that is

    fold (select_cc seteq (and x, y), 0, A, -1) -> (or (sra (shl x)) A)

In other words, transforming the following theoretial C code that
returns -EACCES when certain bit is unset, and -1 when certain bit is
set

    if (fmode & FMODE_WRITE)
        return -1;

    return -EACCESS;

into the following instructions

    r0  <<= 62
    r0 s>>= 63 /* set => r0 = -1, unset => r0 = 0 */
    r0  |= -13 /* set => r0 = (-1 | -13) = -1, unset => r0 = (0 | -13) = -13 = -EACCESS */
        exit       /* returns either -1 or -EACCESS */

So signed ranged deduction with BPF_OR is probably just a nice-to-have
for now.

Yeah. Let's not complicate the verifier until really necessary.

But I wonder whether we should override shouldFoldSelectWithSingleBitTest()
in the backend to suppress this optimization.
I guess not, since removal of a branch is a good thing.