On Mon, Jul 22, 2024 at 05:48:22PM GMT, Alexei Starovoitov wrote:

On Mon, Jul 22, 2024 at 11:48 AM Eduard Zingerman [off-list ref] wrote:

quoted

On Mon, 2024-07-22 at 20:57 +0800, Shung-Hsi Yu wrote:

[...]

quoted

quoted

As a nitpick, I think that it would be good to have some shortened
version of the derivation in the comments alongside the code.

Agree it would. Will try to add a 2-4 sentence explanation.

quoted

(Maybe with a link to the mailing list).

Adding a link to the mailing list seems out of the usual for comment in
verifier.c though, and it would be quite long. That said, it would be
nice to hint that there exists a more verbose version of the
explanation.

Maybe an explicit "see commit for the full detail" at the end of
the added comment?

Tbh, I find bounds deduction code extremely confusing.
Imho, having lengthy comments there is a good thing.

+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, then test for signed range deducation of both
     BPF_AND and BPF_OR can live in the same file, which would be nice
     as signed range deduction of the two are somewhat symmetric