diff mbox series

[v5,bpf-next,3/4] selftest/bpf: Implement sample UNIX domain socket iterator program.

Message ID 20210812164557.79046-4-kuniyu@amazon.co.jp (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series BPF iterator for UNIX domain socket. | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for bpf-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 10 maintainers not CCed: linux-kselftest@vger.kernel.org ndesaulniers@google.com lmb@cloudflare.com alan.maguire@oracle.com nathan@kernel.org shuah@kernel.org edumazet@google.com revest@chromium.org toke@redhat.com clang-built-linux@googlegroups.com
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning CHECK: Macro argument 'unix_sk' may be better as '(unix_sk)' to avoid precedence issues CHECK: Prefer using the BIT macro WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? WARNING: line length of 108 exceeds 80 columns WARNING: line length of 81 exceeds 80 columns WARNING: line length of 82 exceeds 80 columns WARNING: line length of 90 exceeds 80 columns WARNING: line length of 92 exceeds 80 columns
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Iwashima, Kuniyuki Aug. 12, 2021, 4:45 p.m. UTC
The iterator can output almost the same result compared to /proc/net/unix.
The header line is aligned, and the Inode column uses "%8lu" because "%5lu"
can be easily overflown.

  # cat /sys/fs/bpf/unix
  Num               RefCount Protocol Flags    Type St Inode    Path
  ffff963c06689800: 00000002 00000000 00010000 0001 01    18697 private/defer
  ffff963c7c979c00: 00000002 00000000 00000000 0001 01   598245 @Hello@World@

  # cat /proc/net/unix
  Num       RefCount Protocol Flags    Type St Inode Path
  ffff963c06689800: 00000002 00000000 00010000 0001 01 18697 private/defer
  ffff963c7c979c00: 00000002 00000000 00000000 0001 01 598245 @Hello@World@

Note that this prog requires the patch ([0]) for LLVM code gen.  Thanks to
Yonghong Song for analysing and fixing.

[0] https://reviews.llvm.org/D107483

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
Acked-by: Yonghong Song <yhs@fb.com>
---
 tools/testing/selftests/bpf/README.rst        | 38 +++++++++
 .../selftests/bpf/prog_tests/bpf_iter.c       | 16 ++++
 tools/testing/selftests/bpf/progs/bpf_iter.h  |  8 ++
 .../selftests/bpf/progs/bpf_iter_unix.c       | 77 +++++++++++++++++++
 .../selftests/bpf/progs/bpf_tracing_net.h     |  4 +
 5 files changed, 143 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_unix.c

Comments

Andrii Nakryiko Aug. 13, 2021, 11:25 p.m. UTC | #1
On Thu, Aug 12, 2021 at 9:46 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
>
> The iterator can output almost the same result compared to /proc/net/unix.
> The header line is aligned, and the Inode column uses "%8lu" because "%5lu"
> can be easily overflown.
>
>   # cat /sys/fs/bpf/unix
>   Num               RefCount Protocol Flags    Type St Inode    Path

It's totally my OCD, but why the column name is not aligned with
values? I mean the "Inode" column. It's left aligned, but values
(numbers) are right-aligned? I'd fix that while applying, but I can't
apply due to selftests failures, so please take a look.


>   ffff963c06689800: 00000002 00000000 00010000 0001 01    18697 private/defer
>   ffff963c7c979c00: 00000002 00000000 00000000 0001 01   598245 @Hello@World@
>
>   # cat /proc/net/unix
>   Num       RefCount Protocol Flags    Type St Inode Path
>   ffff963c06689800: 00000002 00000000 00010000 0001 01 18697 private/defer
>   ffff963c7c979c00: 00000002 00000000 00000000 0001 01 598245 @Hello@World@
>
> Note that this prog requires the patch ([0]) for LLVM code gen.  Thanks to
> Yonghong Song for analysing and fixing.
>
> [0] https://reviews.llvm.org/D107483
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> Acked-by: Yonghong Song <yhs@fb.com>
> ---

This selftests breaks test_progs-no_alu32 ([0], the error log is super
long and can freeze browser; it looks like an infinite loop and BPF
verifier just keeps reporting it until it runs out of 1mln
instructions or something). Please check what's going on there, I
can't land it as it is right now.

  [0] https://github.com/kernel-patches/bpf/runs/3326071112?check_suite_focus=true#step:6:124288


>  tools/testing/selftests/bpf/README.rst        | 38 +++++++++
>  .../selftests/bpf/prog_tests/bpf_iter.c       | 16 ++++
>  tools/testing/selftests/bpf/progs/bpf_iter.h  |  8 ++
>  .../selftests/bpf/progs/bpf_iter_unix.c       | 77 +++++++++++++++++++
>  .../selftests/bpf/progs/bpf_tracing_net.h     |  4 +
>  5 files changed, 143 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_unix.c
>

[...]

> +                       /* The name of the abstract UNIX domain socket starts
> +                        * with '\0' and can contain '\0'.  The null bytes
> +                        * should be escaped as done in unix_seq_show().
> +                        */
> +                       int i, len;
> +

no_alu32 variant probably isn't happy about using int for this, it
probably does << 32, >> 32 dance and loses track of actual value in
the loop. You can try using u64 instead.

> +                       len = unix_sk->addr->len - sizeof(short);
> +
> +                       BPF_SEQ_PRINTF(seq, " @");
> +
> +                       /* unix_mkname() tests this upper bound. */
> +                       if (len < sizeof(struct sockaddr_un))
> +                               for (i = 1; i < len; i++)

if you move above if inside the loop to break out of the loop, does it
change how Clang generates code?

for (i = 1; i < len i++) {
    if (i >= sizeof(struct sockaddr_un))
        break;
    BPF_SEQ_PRINTF(...);
}


> +                                       BPF_SEQ_PRINTF(seq, "%c",
> +                                                      unix_sk->addr->name->sun_path[i] ?:
> +                                                      '@');
> +               }
> +       }
> +
> +       BPF_SEQ_PRINTF(seq, "\n");
> +
> +       return 0;
> +}
> diff --git a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> index 3af0998a0623..eef5646ddb19 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> +++ b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> @@ -5,6 +5,10 @@
>  #define AF_INET                        2
>  #define AF_INET6               10
>
> +#define __SO_ACCEPTCON         (1 << 16)
> +#define UNIX_HASH_SIZE         256
> +#define UNIX_ABSTRACT(unix_sk) (unix_sk->addr->hash < UNIX_HASH_SIZE)
> +
>  #define SOL_TCP                        6
>  #define TCP_CONGESTION         13
>  #define TCP_CA_NAME_MAX                16
> --
> 2.30.2
>
Iwashima, Kuniyuki Aug. 14, 2021, 12:21 a.m. UTC | #2
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Fri, 13 Aug 2021 16:25:53 -0700
> On Thu, Aug 12, 2021 at 9:46 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
> >
> > The iterator can output almost the same result compared to /proc/net/unix.
> > The header line is aligned, and the Inode column uses "%8lu" because "%5lu"
> > can be easily overflown.
> >
> >   # cat /sys/fs/bpf/unix
> >   Num               RefCount Protocol Flags    Type St Inode    Path
> 
> It's totally my OCD, but why the column name is not aligned with
> values? I mean the "Inode" column. It's left aligned, but values
> (numbers) are right-aligned? I'd fix that while applying, but I can't
> apply due to selftests failures, so please take a look.

Ah, honestly, I've felt something strange about the column... will fix it!


> 
> 
> >   ffff963c06689800: 00000002 00000000 00010000 0001 01    18697 private/defer
> >   ffff963c7c979c00: 00000002 00000000 00000000 0001 01   598245 @Hello@World@
> >
> >   # cat /proc/net/unix
> >   Num       RefCount Protocol Flags    Type St Inode Path
> >   ffff963c06689800: 00000002 00000000 00010000 0001 01 18697 private/defer
> >   ffff963c7c979c00: 00000002 00000000 00000000 0001 01 598245 @Hello@World@
> >
> > Note that this prog requires the patch ([0]) for LLVM code gen.  Thanks to
> > Yonghong Song for analysing and fixing.
> >
> > [0] https://reviews.llvm.org/D107483
> >
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> > Acked-by: Yonghong Song <yhs@fb.com>
> > ---
> 
> This selftests breaks test_progs-no_alu32 ([0], the error log is super
> long and can freeze browser; it looks like an infinite loop and BPF
> verifier just keeps reporting it until it runs out of 1mln
> instructions or something). Please check what's going on there, I
> can't land it as it is right now.
> 
>   [0] https://github.com/kernel-patches/bpf/runs/3326071112?check_suite_focus=true#step:6:124288
> 
> 
> >  tools/testing/selftests/bpf/README.rst        | 38 +++++++++
> >  .../selftests/bpf/prog_tests/bpf_iter.c       | 16 ++++
> >  tools/testing/selftests/bpf/progs/bpf_iter.h  |  8 ++
> >  .../selftests/bpf/progs/bpf_iter_unix.c       | 77 +++++++++++++++++++
> >  .../selftests/bpf/progs/bpf_tracing_net.h     |  4 +
> >  5 files changed, 143 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_unix.c
> >
> 
> [...]
> 
> > +                       /* The name of the abstract UNIX domain socket starts
> > +                        * with '\0' and can contain '\0'.  The null bytes
> > +                        * should be escaped as done in unix_seq_show().
> > +                        */
> > +                       int i, len;
> > +
> 
> no_alu32 variant probably isn't happy about using int for this, it
> probably does << 32, >> 32 dance and loses track of actual value in
> the loop. You can try using u64 instead.

Sorry, I missed the no_alu32 test.
Changing int to __u64 fixed the error, thanks!


> 
> > +                       len = unix_sk->addr->len - sizeof(short);
> > +
> > +                       BPF_SEQ_PRINTF(seq, " @");
> > +
> > +                       /* unix_mkname() tests this upper bound. */
> > +                       if (len < sizeof(struct sockaddr_un))
> > +                               for (i = 1; i < len; i++)
> 
> if you move above if inside the loop to break out of the loop, does it
> change how Clang generates code?
> 
> for (i = 1; i < len i++) {
>     if (i >= sizeof(struct sockaddr_un))
>         break;
>     BPF_SEQ_PRINTF(...);
> }

Yes, but there seems little defference.
Which is preferable?

---8<---
before (for inside if) <- -> after (if inside loop)
      96:	07 08 00 00 fe ff ff ff	r8 += -2			  |	; 			for (i = 1; i < len; i++) {
; 			if (len < sizeof(struct sockaddr_un))		  |	      97:	bf 81 00 00 00 00 00 00	r1 = r8
      97:	25 08 10 00 6d 00 00 00	if r8 > 109 goto +16 <LBB0_21>	  |	      98:	07 01 00 00 fc ff ff ff	r1 += -4
; 				for (i = 1; i < len; i++)		  |	      99:	25 01 12 00 6b 00 00 00	if r1 > 107 goto +18 <LBB0_21>
      98:	a5 08 0f 00 02 00 00 00	if r8 < 2 goto +15 <LBB0_21>	  |	     100:	07 08 00 00 fe ff ff ff	r8 += -2
      99:	b7 09 00 00 01 00 00 00	r9 = 1				  |	     101:	b7 09 00 00 01 00 00 00	r9 = 1
     100:	05 00 16 00 00 00 00 00	goto +22 <LBB0_18>		  |	     102:	b7 06 00 00 02 00 00 00	r6 = 2
									  |	     103:	05 00 17 00 00 00 00 00	goto +23 <LBB0_17>
...
     111:	85 00 00 00 7e 00 00 00	call 126			  |	     113:	b4 05 00 00 08 00 00 00	w5 = 8
; 				for (i = 1; i < len; i++)		  |	     114:	85 00 00 00 7e 00 00 00	call 126
     112:	07 09 00 00 01 00 00 00	r9 += 1				  |	; 			for (i = 1; i < len; i++) {
     113:	ad 89 09 00 00 00 00 00	if r9 < r8 goto +9 <LBB0_18>	  |	     115:	25 08 02 00 6d 00 00 00	if r8 > 109 goto +2 <LBB0_21>
									  >	     116:	07 09 00 00 01 00 00 00	r9 += 1
									  >	; 			for (i = 1; i < len; i++) {
									  >	     117:	ad 89 09 00 00 00 00 00	if r9 < r8 goto +9 <LBB0_17>
---8<---


> 
> 
> > +                                       BPF_SEQ_PRINTF(seq, "%c",
> > +                                                      unix_sk->addr->name->sun_path[i] ?:
> > +                                                      '@');
> > +               }
> > +       }
> > +
> > +       BPF_SEQ_PRINTF(seq, "\n");
> > +
> > +       return 0;
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> > index 3af0998a0623..eef5646ddb19 100644
> > --- a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> > +++ b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> > @@ -5,6 +5,10 @@
> >  #define AF_INET                        2
> >  #define AF_INET6               10
> >
> > +#define __SO_ACCEPTCON         (1 << 16)
> > +#define UNIX_HASH_SIZE         256
> > +#define UNIX_ABSTRACT(unix_sk) (unix_sk->addr->hash < UNIX_HASH_SIZE)
> > +
> >  #define SOL_TCP                        6
> >  #define TCP_CONGESTION         13
> >  #define TCP_CA_NAME_MAX                16
> > --
> > 2.30.2
> >
Andrii Nakryiko Aug. 14, 2021, 12:26 a.m. UTC | #3
On Fri, Aug 13, 2021 at 5:21 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
>
> From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Date:   Fri, 13 Aug 2021 16:25:53 -0700
> > On Thu, Aug 12, 2021 at 9:46 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
> > >
> > > The iterator can output almost the same result compared to /proc/net/unix.
> > > The header line is aligned, and the Inode column uses "%8lu" because "%5lu"
> > > can be easily overflown.
> > >
> > >   # cat /sys/fs/bpf/unix
> > >   Num               RefCount Protocol Flags    Type St Inode    Path
> >
> > It's totally my OCD, but why the column name is not aligned with
> > values? I mean the "Inode" column. It's left aligned, but values
> > (numbers) are right-aligned? I'd fix that while applying, but I can't
> > apply due to selftests failures, so please take a look.
>
> Ah, honestly, I've felt something strange about the column... will fix it!
>
>
> >
> >
> > >   ffff963c06689800: 00000002 00000000 00010000 0001 01    18697 private/defer
> > >   ffff963c7c979c00: 00000002 00000000 00000000 0001 01   598245 @Hello@World@
> > >
> > >   # cat /proc/net/unix
> > >   Num       RefCount Protocol Flags    Type St Inode Path
> > >   ffff963c06689800: 00000002 00000000 00010000 0001 01 18697 private/defer
> > >   ffff963c7c979c00: 00000002 00000000 00000000 0001 01 598245 @Hello@World@
> > >
> > > Note that this prog requires the patch ([0]) for LLVM code gen.  Thanks to
> > > Yonghong Song for analysing and fixing.
> > >
> > > [0] https://reviews.llvm.org/D107483
> > >
> > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> > > Acked-by: Yonghong Song <yhs@fb.com>
> > > ---
> >
> > This selftests breaks test_progs-no_alu32 ([0], the error log is super
> > long and can freeze browser; it looks like an infinite loop and BPF
> > verifier just keeps reporting it until it runs out of 1mln
> > instructions or something). Please check what's going on there, I
> > can't land it as it is right now.
> >
> >   [0] https://github.com/kernel-patches/bpf/runs/3326071112?check_suite_focus=true#step:6:124288
> >
> >
> > >  tools/testing/selftests/bpf/README.rst        | 38 +++++++++
> > >  .../selftests/bpf/prog_tests/bpf_iter.c       | 16 ++++
> > >  tools/testing/selftests/bpf/progs/bpf_iter.h  |  8 ++
> > >  .../selftests/bpf/progs/bpf_iter_unix.c       | 77 +++++++++++++++++++
> > >  .../selftests/bpf/progs/bpf_tracing_net.h     |  4 +
> > >  5 files changed, 143 insertions(+)
> > >  create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_unix.c
> > >
> >
> > [...]
> >
> > > +                       /* The name of the abstract UNIX domain socket starts
> > > +                        * with '\0' and can contain '\0'.  The null bytes
> > > +                        * should be escaped as done in unix_seq_show().
> > > +                        */
> > > +                       int i, len;
> > > +
> >
> > no_alu32 variant probably isn't happy about using int for this, it
> > probably does << 32, >> 32 dance and loses track of actual value in
> > the loop. You can try using u64 instead.
>
> Sorry, I missed the no_alu32 test.
> Changing int to __u64 fixed the error, thanks!
>
>
> >
> > > +                       len = unix_sk->addr->len - sizeof(short);
> > > +
> > > +                       BPF_SEQ_PRINTF(seq, " @");
> > > +
> > > +                       /* unix_mkname() tests this upper bound. */
> > > +                       if (len < sizeof(struct sockaddr_un))
> > > +                               for (i = 1; i < len; i++)
> >
> > if you move above if inside the loop to break out of the loop, does it
> > change how Clang generates code?
> >
> > for (i = 1; i < len i++) {
> >     if (i >= sizeof(struct sockaddr_un))
> >         break;
> >     BPF_SEQ_PRINTF(...);
> > }
>
> Yes, but there seems little defference.
> Which is preferable?
>
> ---8<---
> before (for inside if) <- -> after (if inside loop)
>       96:       07 08 00 00 fe ff ff ff r8 += -2                          |     ;                       for (i = 1; i < len; i++) {
> ;                       if (len < sizeof(struct sockaddr_un))             |           97:       bf 81 00 00 00 00 00 00 r1 = r8
>       97:       25 08 10 00 6d 00 00 00 if r8 > 109 goto +16 <LBB0_21>    |           98:       07 01 00 00 fc ff ff ff r1 += -4
> ;                               for (i = 1; i < len; i++)                 |           99:       25 01 12 00 6b 00 00 00 if r1 > 107 goto +18 <LBB0_21>
>       98:       a5 08 0f 00 02 00 00 00 if r8 < 2 goto +15 <LBB0_21>      |          100:       07 08 00 00 fe ff ff ff r8 += -2
>       99:       b7 09 00 00 01 00 00 00 r9 = 1                            |          101:       b7 09 00 00 01 00 00 00 r9 = 1
>      100:       05 00 16 00 00 00 00 00 goto +22 <LBB0_18>                |          102:       b7 06 00 00 02 00 00 00 r6 = 2
>                                                                           |          103:       05 00 17 00 00 00 00 00 goto +23 <LBB0_17>
> ...
>      111:       85 00 00 00 7e 00 00 00 call 126                          |          113:       b4 05 00 00 08 00 00 00 w5 = 8
> ;                               for (i = 1; i < len; i++)                 |          114:       85 00 00 00 7e 00 00 00 call 126
>      112:       07 09 00 00 01 00 00 00 r9 += 1                           |     ;                       for (i = 1; i < len; i++) {
>      113:       ad 89 09 00 00 00 00 00 if r9 < r8 goto +9 <LBB0_18>      |          115:       25 08 02 00 6d 00 00 00 if r8 > 109 goto +2 <LBB0_21>
>                                                                           >          116:       07 09 00 00 01 00 00 00 r9 += 1
>                                                                           >     ;                       for (i = 1; i < len; i++) {
>                                                                           >          117:       ad 89 09 00 00 00 00 00 if r9 < r8 goto +9 <LBB0_17>
> ---8<---
>

Have you tried running the variant I proposed on Clang without
Yonghong's recent fix? I wonder if it works without that fix (not that
there is anything wrong about the fix, but if we can avoid depending
on it, it would be great).

>
> >
> >
> > > +                                       BPF_SEQ_PRINTF(seq, "%c",
> > > +                                                      unix_sk->addr->name->sun_path[i] ?:
> > > +                                                      '@');
> > > +               }
> > > +       }
> > > +
> > > +       BPF_SEQ_PRINTF(seq, "\n");
> > > +
> > > +       return 0;
> > > +}
> > > diff --git a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> > > index 3af0998a0623..eef5646ddb19 100644
> > > --- a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> > > +++ b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> > > @@ -5,6 +5,10 @@
> > >  #define AF_INET                        2
> > >  #define AF_INET6               10
> > >
> > > +#define __SO_ACCEPTCON         (1 << 16)
> > > +#define UNIX_HASH_SIZE         256
> > > +#define UNIX_ABSTRACT(unix_sk) (unix_sk->addr->hash < UNIX_HASH_SIZE)
> > > +
> > >  #define SOL_TCP                        6
> > >  #define TCP_CONGESTION         13
> > >  #define TCP_CA_NAME_MAX                16
> > > --
> > > 2.30.2
> > >
Iwashima, Kuniyuki Aug. 14, 2021, 1 a.m. UTC | #4
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Fri, 13 Aug 2021 17:26:05 -0700
> On Fri, Aug 13, 2021 at 5:21 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
> >
> > From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > Date:   Fri, 13 Aug 2021 16:25:53 -0700
> > > On Thu, Aug 12, 2021 at 9:46 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
> > > >
> > > > The iterator can output almost the same result compared to /proc/net/unix.
> > > > The header line is aligned, and the Inode column uses "%8lu" because "%5lu"
> > > > can be easily overflown.
> > > >
> > > >   # cat /sys/fs/bpf/unix
> > > >   Num               RefCount Protocol Flags    Type St Inode    Path
> > >
> > > It's totally my OCD, but why the column name is not aligned with
> > > values? I mean the "Inode" column. It's left aligned, but values
> > > (numbers) are right-aligned? I'd fix that while applying, but I can't
> > > apply due to selftests failures, so please take a look.
> >
> > Ah, honestly, I've felt something strange about the column... will fix it!
> >
> >
> > >
> > >
> > > >   ffff963c06689800: 00000002 00000000 00010000 0001 01    18697 private/defer
> > > >   ffff963c7c979c00: 00000002 00000000 00000000 0001 01   598245 @Hello@World@
> > > >
> > > >   # cat /proc/net/unix
> > > >   Num       RefCount Protocol Flags    Type St Inode Path
> > > >   ffff963c06689800: 00000002 00000000 00010000 0001 01 18697 private/defer
> > > >   ffff963c7c979c00: 00000002 00000000 00000000 0001 01 598245 @Hello@World@
> > > >
> > > > Note that this prog requires the patch ([0]) for LLVM code gen.  Thanks to
> > > > Yonghong Song for analysing and fixing.
> > > >
> > > > [0] https://reviews.llvm.org/D107483
> > > >
> > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> > > > Acked-by: Yonghong Song <yhs@fb.com>
> > > > ---
> > >
> > > This selftests breaks test_progs-no_alu32 ([0], the error log is super
> > > long and can freeze browser; it looks like an infinite loop and BPF
> > > verifier just keeps reporting it until it runs out of 1mln
> > > instructions or something). Please check what's going on there, I
> > > can't land it as it is right now.
> > >
> > >   [0] https://github.com/kernel-patches/bpf/runs/3326071112?check_suite_focus=true#step:6:124288
> > >
> > >
> > > >  tools/testing/selftests/bpf/README.rst        | 38 +++++++++
> > > >  .../selftests/bpf/prog_tests/bpf_iter.c       | 16 ++++
> > > >  tools/testing/selftests/bpf/progs/bpf_iter.h  |  8 ++
> > > >  .../selftests/bpf/progs/bpf_iter_unix.c       | 77 +++++++++++++++++++
> > > >  .../selftests/bpf/progs/bpf_tracing_net.h     |  4 +
> > > >  5 files changed, 143 insertions(+)
> > > >  create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_unix.c
> > > >
> > >
> > > [...]
> > >
> > > > +                       /* The name of the abstract UNIX domain socket starts
> > > > +                        * with '\0' and can contain '\0'.  The null bytes
> > > > +                        * should be escaped as done in unix_seq_show().
> > > > +                        */
> > > > +                       int i, len;
> > > > +
> > >
> > > no_alu32 variant probably isn't happy about using int for this, it
> > > probably does << 32, >> 32 dance and loses track of actual value in
> > > the loop. You can try using u64 instead.
> >
> > Sorry, I missed the no_alu32 test.
> > Changing int to __u64 fixed the error, thanks!
> >
> >
> > >
> > > > +                       len = unix_sk->addr->len - sizeof(short);
> > > > +
> > > > +                       BPF_SEQ_PRINTF(seq, " @");
> > > > +
> > > > +                       /* unix_mkname() tests this upper bound. */
> > > > +                       if (len < sizeof(struct sockaddr_un))
> > > > +                               for (i = 1; i < len; i++)
> > >
> > > if you move above if inside the loop to break out of the loop, does it
> > > change how Clang generates code?
> > >
> > > for (i = 1; i < len i++) {
> > >     if (i >= sizeof(struct sockaddr_un))
> > >         break;
> > >     BPF_SEQ_PRINTF(...);
> > > }
> >
> > Yes, but there seems little defference.
> > Which is preferable?
> >
> > ---8<---
> > before (for inside if) <- -> after (if inside loop)
> >       96:       07 08 00 00 fe ff ff ff r8 += -2                          |     ;                       for (i = 1; i < len; i++) {
> > ;                       if (len < sizeof(struct sockaddr_un))             |           97:       bf 81 00 00 00 00 00 00 r1 = r8
> >       97:       25 08 10 00 6d 00 00 00 if r8 > 109 goto +16 <LBB0_21>    |           98:       07 01 00 00 fc ff ff ff r1 += -4
> > ;                               for (i = 1; i < len; i++)                 |           99:       25 01 12 00 6b 00 00 00 if r1 > 107 goto +18 <LBB0_21>
> >       98:       a5 08 0f 00 02 00 00 00 if r8 < 2 goto +15 <LBB0_21>      |          100:       07 08 00 00 fe ff ff ff r8 += -2
> >       99:       b7 09 00 00 01 00 00 00 r9 = 1                            |          101:       b7 09 00 00 01 00 00 00 r9 = 1
> >      100:       05 00 16 00 00 00 00 00 goto +22 <LBB0_18>                |          102:       b7 06 00 00 02 00 00 00 r6 = 2
> >                                                                           |          103:       05 00 17 00 00 00 00 00 goto +23 <LBB0_17>
> > ...
> >      111:       85 00 00 00 7e 00 00 00 call 126                          |          113:       b4 05 00 00 08 00 00 00 w5 = 8
> > ;                               for (i = 1; i < len; i++)                 |          114:       85 00 00 00 7e 00 00 00 call 126
> >      112:       07 09 00 00 01 00 00 00 r9 += 1                           |     ;                       for (i = 1; i < len; i++) {
> >      113:       ad 89 09 00 00 00 00 00 if r9 < r8 goto +9 <LBB0_18>      |          115:       25 08 02 00 6d 00 00 00 if r8 > 109 goto +2 <LBB0_21>
> >                                                                           >          116:       07 09 00 00 01 00 00 00 r9 += 1
> >                                                                           >     ;                       for (i = 1; i < len; i++) {
> >                                                                           >          117:       ad 89 09 00 00 00 00 00 if r9 < r8 goto +9 <LBB0_17>
> > ---8<---
> >
> 
> Have you tried running the variant I proposed on Clang without
> Yonghong's recent fix? I wonder if it works without that fix (not that
> there is anything wrong about the fix, but if we can avoid depending
> on it, it would be great).

It was with the fix.

I rebuilt LLVM without the fix, then the if-inside-for code worked well :)
There was no transformation from '<' to '!='.

I'll drop the change in README and respin with your suggestion soon.

---8<---
; 			for (i = 1; i < len; i++) {
      97:	bf 81 00 00 00 00 00 00	r1 = r8
      98:	07 01 00 00 fc ff ff ff	r1 += -4
      99:	25 01 12 00 6b 00 00 00	if r1 > 107 goto +18 <LBB0_21>
     100:	07 08 00 00 fe ff ff ff	r8 += -2
     101:	b7 09 00 00 01 00 00 00	r9 = 1
     102:	b7 06 00 00 02 00 00 00	r6 = 2
     103:	05 00 17 00 00 00 00 00	goto +23 <LBB0_17>
...
; 			for (i = 1; i < len; i++) {
     115:	25 08 02 00 6d 00 00 00	if r8 > 109 goto +2 <LBB0_21>
     116:	07 09 00 00 01 00 00 00	r9 += 1
; 			for (i = 1; i < len; i++) {
     117:	ad 89 09 00 00 00 00 00	if r9 < r8 goto +9 <LBB0_17>
---8<---


> 
> >
> > >
> > >
> > > > +                                       BPF_SEQ_PRINTF(seq, "%c",
> > > > +                                                      unix_sk->addr->name->sun_path[i] ?:
> > > > +                                                      '@');
> > > > +               }
> > > > +       }
> > > > +
> > > > +       BPF_SEQ_PRINTF(seq, "\n");
> > > > +
> > > > +       return 0;
> > > > +}
> > > > diff --git a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> > > > index 3af0998a0623..eef5646ddb19 100644
> > > > --- a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> > > > +++ b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> > > > @@ -5,6 +5,10 @@
> > > >  #define AF_INET                        2
> > > >  #define AF_INET6               10
> > > >
> > > > +#define __SO_ACCEPTCON         (1 << 16)
> > > > +#define UNIX_HASH_SIZE         256
> > > > +#define UNIX_ABSTRACT(unix_sk) (unix_sk->addr->hash < UNIX_HASH_SIZE)
> > > > +
> > > >  #define SOL_TCP                        6
> > > >  #define TCP_CONGESTION         13
> > > >  #define TCP_CA_NAME_MAX                16
> > > > --
> > > > 2.30.2
Yonghong Song Aug. 15, 2021, 6:10 p.m. UTC | #5
On 8/13/21 5:21 PM, Kuniyuki Iwashima wrote:
> From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Date:   Fri, 13 Aug 2021 16:25:53 -0700
>> On Thu, Aug 12, 2021 at 9:46 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
>>>
>>> The iterator can output almost the same result compared to /proc/net/unix.
>>> The header line is aligned, and the Inode column uses "%8lu" because "%5lu"
>>> can be easily overflown.
>>>
>>>    # cat /sys/fs/bpf/unix
>>>    Num               RefCount Protocol Flags    Type St Inode    Path
>>
>> It's totally my OCD, but why the column name is not aligned with
>> values? I mean the "Inode" column. It's left aligned, but values
>> (numbers) are right-aligned? I'd fix that while applying, but I can't
>> apply due to selftests failures, so please take a look.
> 
> Ah, honestly, I've felt something strange about the column... will fix it!
> 
> 
>>
>>
>>>    ffff963c06689800: 00000002 00000000 00010000 0001 01    18697 private/defer
>>>    ffff963c7c979c00: 00000002 00000000 00000000 0001 01   598245 @Hello@World@
>>>
>>>    # cat /proc/net/unix
>>>    Num       RefCount Protocol Flags    Type St Inode Path
>>>    ffff963c06689800: 00000002 00000000 00010000 0001 01 18697 private/defer
>>>    ffff963c7c979c00: 00000002 00000000 00000000 0001 01 598245 @Hello@World@
>>>
>>> Note that this prog requires the patch ([0]) for LLVM code gen.  Thanks to
>>> Yonghong Song for analysing and fixing.
>>>
>>> [0] https://reviews.llvm.org/D107483
>>>
>>> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
>>> Acked-by: Yonghong Song <yhs@fb.com>
>>> ---
>>
>> This selftests breaks test_progs-no_alu32 ([0], the error log is super
>> long and can freeze browser; it looks like an infinite loop and BPF
>> verifier just keeps reporting it until it runs out of 1mln
>> instructions or something). Please check what's going on there, I
>> can't land it as it is right now.
>>
>>    [0] https://github.com/kernel-patches/bpf/runs/3326071112?check_suite_focus=true#step:6:124288
>>
>>
>>>   tools/testing/selftests/bpf/README.rst        | 38 +++++++++
>>>   .../selftests/bpf/prog_tests/bpf_iter.c       | 16 ++++
>>>   tools/testing/selftests/bpf/progs/bpf_iter.h  |  8 ++
>>>   .../selftests/bpf/progs/bpf_iter_unix.c       | 77 +++++++++++++++++++
>>>   .../selftests/bpf/progs/bpf_tracing_net.h     |  4 +
>>>   5 files changed, 143 insertions(+)
>>>   create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_unix.c
>>>
>>
>> [...]
>>
>>> +                       /* The name of the abstract UNIX domain socket starts
>>> +                        * with '\0' and can contain '\0'.  The null bytes
>>> +                        * should be escaped as done in unix_seq_show().
>>> +                        */
>>> +                       int i, len;
>>> +
>>
>> no_alu32 variant probably isn't happy about using int for this, it
>> probably does << 32, >> 32 dance and loses track of actual value in
>> the loop. You can try using u64 instead.
> 
> Sorry, I missed the no_alu32 test.
> Changing int to __u64 fixed the error, thanks!

Indeed for no_alu32, the index has << 32 and >> 32, which makes
verifier *equivalent* register tracking not effective, see below:

       96:       r1 = r8 

       97:       r1 <<= 32 

       98:       r2 = r1 

       99:       r2 >>= 32 

      100:       if r2 > 109 goto +19 <LBB0_21> 

      101:       r1 s>>= 32 

      102:       if r1 s< 2 goto +17 <LBB0_21> 

      103:       r9 = 1 

      104:       r8 <<= 32 

      105:       r8 >>= 32

Because these shifting, r1/r2/r8 equivalence cannot be
easily established, so verifier ends with conservative
r8 and cannot verify program successfully.

Using __u64 for 'i' and 'len', the upper bound is directly
tested:
       98:       if r8 > 109 goto +16 <LBB0_21> 

       99:       if r8 < 2 goto +15 <LBB0_21>
and verifier is very happy with this.

> 
> 
>>
>>> +                       len = unix_sk->addr->len - sizeof(short);
>>> +
>>> +                       BPF_SEQ_PRINTF(seq, " @");
>>> +
>>> +                       /* unix_mkname() tests this upper bound. */
>>> +                       if (len < sizeof(struct sockaddr_un))
>>> +                               for (i = 1; i < len; i++)
>>
>> if you move above if inside the loop to break out of the loop, does it
>> change how Clang generates code?
>>
>> for (i = 1; i < len i++) {
>>      if (i >= sizeof(struct sockaddr_un))
>>          break;
>>      BPF_SEQ_PRINTF(...);
>> }
> 
> Yes, but there seems little defference.
> Which is preferable?
> 
> ---8<---
> before (for inside if) <- -> after (if inside loop)
>        96:	07 08 00 00 fe ff ff ff	r8 += -2			  |	; 			for (i = 1; i < len; i++) {
> ; 			if (len < sizeof(struct sockaddr_un))		  |	      97:	bf 81 00 00 00 00 00 00	r1 = r8
>        97:	25 08 10 00 6d 00 00 00	if r8 > 109 goto +16 <LBB0_21>	  |	      98:	07 01 00 00 fc ff ff ff	r1 += -4
> ; 				for (i = 1; i < len; i++)		  |	      99:	25 01 12 00 6b 00 00 00	if r1 > 107 goto +18 <LBB0_21>
>        98:	a5 08 0f 00 02 00 00 00	if r8 < 2 goto +15 <LBB0_21>	  |	     100:	07 08 00 00 fe ff ff ff	r8 += -2
>        99:	b7 09 00 00 01 00 00 00	r9 = 1				  |	     101:	b7 09 00 00 01 00 00 00	r9 = 1
>       100:	05 00 16 00 00 00 00 00	goto +22 <LBB0_18>		  |	     102:	b7 06 00 00 02 00 00 00	r6 = 2
> 									  |	     103:	05 00 17 00 00 00 00 00	goto +23 <LBB0_17>
> ...
>       111:	85 00 00 00 7e 00 00 00	call 126			  |	     113:	b4 05 00 00 08 00 00 00	w5 = 8
> ; 				for (i = 1; i < len; i++)		  |	     114:	85 00 00 00 7e 00 00 00	call 126
>       112:	07 09 00 00 01 00 00 00	r9 += 1				  |	; 			for (i = 1; i < len; i++) {
>       113:	ad 89 09 00 00 00 00 00	if r9 < r8 goto +9 <LBB0_18>	  |	     115:	25 08 02 00 6d 00 00 00	if r8 > 109 goto +2 <LBB0_21>
> 									  >	     116:	07 09 00 00 01 00 00 00	r9 += 1
> 									  >	; 			for (i = 1; i < len; i++) {
> 									  >	     117:	ad 89 09 00 00 00 00 00	if r9 < r8 goto +9 <LBB0_17>
> ---8<---
> 
> 
>>
>>
>>> +                                       BPF_SEQ_PRINTF(seq, "%c",
>>> +                                                      unix_sk->addr->name->sun_path[i] ?:
>>> +                                                      '@');
>>> +               }
>>> +       }
>>> +
>>> +       BPF_SEQ_PRINTF(seq, "\n");
>>> +
>>> +       return 0;
>>> +}
>>> diff --git a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
>>> index 3af0998a0623..eef5646ddb19 100644
>>> --- a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
>>> +++ b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
>>> @@ -5,6 +5,10 @@
>>>   #define AF_INET                        2
>>>   #define AF_INET6               10
>>>
>>> +#define __SO_ACCEPTCON         (1 << 16)
>>> +#define UNIX_HASH_SIZE         256
>>> +#define UNIX_ABSTRACT(unix_sk) (unix_sk->addr->hash < UNIX_HASH_SIZE)
>>> +
>>>   #define SOL_TCP                        6
>>>   #define TCP_CONGESTION         13
>>>   #define TCP_CA_NAME_MAX                16
>>> --
>>> 2.30.2
>>>
Iwashima, Kuniyuki Aug. 16, 2021, 12:45 p.m. UTC | #6
From:   Yonghong Song <yhs@fb.com>
Date:   Sun, 15 Aug 2021 11:10:49 -0700
> On 8/13/21 5:21 PM, Kuniyuki Iwashima wrote:
> > From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > Date:   Fri, 13 Aug 2021 16:25:53 -0700
> >> On Thu, Aug 12, 2021 at 9:46 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
> >>>
> >>> The iterator can output almost the same result compared to /proc/net/unix.
> >>> The header line is aligned, and the Inode column uses "%8lu" because "%5lu"
> >>> can be easily overflown.
> >>>
> >>>    # cat /sys/fs/bpf/unix
> >>>    Num               RefCount Protocol Flags    Type St Inode    Path
> >>
> >> It's totally my OCD, but why the column name is not aligned with
> >> values? I mean the "Inode" column. It's left aligned, but values
> >> (numbers) are right-aligned? I'd fix that while applying, but I can't
> >> apply due to selftests failures, so please take a look.
> > 
> > Ah, honestly, I've felt something strange about the column... will fix it!
> > 
> > 
> >>
> >>
> >>>    ffff963c06689800: 00000002 00000000 00010000 0001 01    18697 private/defer
> >>>    ffff963c7c979c00: 00000002 00000000 00000000 0001 01   598245 @Hello@World@
> >>>
> >>>    # cat /proc/net/unix
> >>>    Num       RefCount Protocol Flags    Type St Inode Path
> >>>    ffff963c06689800: 00000002 00000000 00010000 0001 01 18697 private/defer
> >>>    ffff963c7c979c00: 00000002 00000000 00000000 0001 01 598245 @Hello@World@
> >>>
> >>> Note that this prog requires the patch ([0]) for LLVM code gen.  Thanks to
> >>> Yonghong Song for analysing and fixing.
> >>>
> >>> [0] https://reviews.llvm.org/D107483
> >>>
> >>> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> >>> Acked-by: Yonghong Song <yhs@fb.com>
> >>> ---
> >>
> >> This selftests breaks test_progs-no_alu32 ([0], the error log is super
> >> long and can freeze browser; it looks like an infinite loop and BPF
> >> verifier just keeps reporting it until it runs out of 1mln
> >> instructions or something). Please check what's going on there, I
> >> can't land it as it is right now.
> >>
> >>    [0] https://github.com/kernel-patches/bpf/runs/3326071112?check_suite_focus=true#step:6:124288
> >>
> >>
> >>>   tools/testing/selftests/bpf/README.rst        | 38 +++++++++
> >>>   .../selftests/bpf/prog_tests/bpf_iter.c       | 16 ++++
> >>>   tools/testing/selftests/bpf/progs/bpf_iter.h  |  8 ++
> >>>   .../selftests/bpf/progs/bpf_iter_unix.c       | 77 +++++++++++++++++++
> >>>   .../selftests/bpf/progs/bpf_tracing_net.h     |  4 +
> >>>   5 files changed, 143 insertions(+)
> >>>   create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_unix.c
> >>>
> >>
> >> [...]
> >>
> >>> +                       /* The name of the abstract UNIX domain socket starts
> >>> +                        * with '\0' and can contain '\0'.  The null bytes
> >>> +                        * should be escaped as done in unix_seq_show().
> >>> +                        */
> >>> +                       int i, len;
> >>> +
> >>
> >> no_alu32 variant probably isn't happy about using int for this, it
> >> probably does << 32, >> 32 dance and loses track of actual value in
> >> the loop. You can try using u64 instead.
> > 
> > Sorry, I missed the no_alu32 test.
> > Changing int to __u64 fixed the error, thanks!
> 
> Indeed for no_alu32, the index has << 32 and >> 32, which makes
> verifier *equivalent* register tracking not effective, see below:
> 
>        96:       r1 = r8 
> 
>        97:       r1 <<= 32 
> 
>        98:       r2 = r1 
> 
>        99:       r2 >>= 32 
> 
>       100:       if r2 > 109 goto +19 <LBB0_21> 
> 
>       101:       r1 s>>= 32 
> 
>       102:       if r1 s< 2 goto +17 <LBB0_21> 
> 
>       103:       r9 = 1 
> 
>       104:       r8 <<= 32 
> 
>       105:       r8 >>= 32
> 
> Because these shifting, r1/r2/r8 equivalence cannot be
> easily established, so verifier ends with conservative
> r8 and cannot verify program successfully.
> 
> Using __u64 for 'i' and 'len', the upper bound is directly
> tested:
>        98:       if r8 > 109 goto +16 <LBB0_21> 
> 
>        99:       if r8 < 2 goto +15 <LBB0_21>
> and verifier is very happy with this.

Thanks for explanation!

I understand that the shift dance is to mimic the overflow of int because
actually 64-bit register is allocated to 'i' and 32-bit operations cannot
be used in no_alu32 test, so using __64 to remove the dance resolves it.


> 
> > 
> > 
> >>
> >>> +                       len = unix_sk->addr->len - sizeof(short);
> >>> +
> >>> +                       BPF_SEQ_PRINTF(seq, " @");
> >>> +
> >>> +                       /* unix_mkname() tests this upper bound. */
> >>> +                       if (len < sizeof(struct sockaddr_un))
> >>> +                               for (i = 1; i < len; i++)
> >>
> >> if you move above if inside the loop to break out of the loop, does it
> >> change how Clang generates code?
> >>
> >> for (i = 1; i < len i++) {
> >>      if (i >= sizeof(struct sockaddr_un))
> >>          break;
> >>      BPF_SEQ_PRINTF(...);
> >> }
> > 
> > Yes, but there seems little defference.
> > Which is preferable?
> > 
> > ---8<---
> > before (for inside if) <- -> after (if inside loop)
> >        96:	07 08 00 00 fe ff ff ff	r8 += -2			  |	; 			for (i = 1; i < len; i++) {
> > ; 			if (len < sizeof(struct sockaddr_un))		  |	      97:	bf 81 00 00 00 00 00 00	r1 = r8
> >        97:	25 08 10 00 6d 00 00 00	if r8 > 109 goto +16 <LBB0_21>	  |	      98:	07 01 00 00 fc ff ff ff	r1 += -4
> > ; 				for (i = 1; i < len; i++)		  |	      99:	25 01 12 00 6b 00 00 00	if r1 > 107 goto +18 <LBB0_21>
> >        98:	a5 08 0f 00 02 00 00 00	if r8 < 2 goto +15 <LBB0_21>	  |	     100:	07 08 00 00 fe ff ff ff	r8 += -2
> >        99:	b7 09 00 00 01 00 00 00	r9 = 1				  |	     101:	b7 09 00 00 01 00 00 00	r9 = 1
> >       100:	05 00 16 00 00 00 00 00	goto +22 <LBB0_18>		  |	     102:	b7 06 00 00 02 00 00 00	r6 = 2
> > 									  |	     103:	05 00 17 00 00 00 00 00	goto +23 <LBB0_17>
> > ...
> >       111:	85 00 00 00 7e 00 00 00	call 126			  |	     113:	b4 05 00 00 08 00 00 00	w5 = 8
> > ; 				for (i = 1; i < len; i++)		  |	     114:	85 00 00 00 7e 00 00 00	call 126
> >       112:	07 09 00 00 01 00 00 00	r9 += 1				  |	; 			for (i = 1; i < len; i++) {
> >       113:	ad 89 09 00 00 00 00 00	if r9 < r8 goto +9 <LBB0_18>	  |	     115:	25 08 02 00 6d 00 00 00	if r8 > 109 goto +2 <LBB0_21>
> > 									  >	     116:	07 09 00 00 01 00 00 00	r9 += 1
> > 									  >	; 			for (i = 1; i < len; i++) {
> > 									  >	     117:	ad 89 09 00 00 00 00 00	if r9 < r8 goto +9 <LBB0_17>
> > ---8<---
> > 
> > 
> >>
> >>
> >>> +                                       BPF_SEQ_PRINTF(seq, "%c",
> >>> +                                                      unix_sk->addr->name->sun_path[i] ?:
> >>> +                                                      '@');
> >>> +               }
> >>> +       }
> >>> +
> >>> +       BPF_SEQ_PRINTF(seq, "\n");
> >>> +
> >>> +       return 0;
> >>> +}
> >>> diff --git a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> >>> index 3af0998a0623..eef5646ddb19 100644
> >>> --- a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> >>> +++ b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> >>> @@ -5,6 +5,10 @@
> >>>   #define AF_INET                        2
> >>>   #define AF_INET6               10
> >>>
> >>> +#define __SO_ACCEPTCON         (1 << 16)
> >>> +#define UNIX_HASH_SIZE         256
> >>> +#define UNIX_ABSTRACT(unix_sk) (unix_sk->addr->hash < UNIX_HASH_SIZE)
> >>> +
> >>>   #define SOL_TCP                        6
> >>>   #define TCP_CONGESTION         13
> >>>   #define TCP_CA_NAME_MAX                16
> >>> --
> >>> 2.30.2
> >>>
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/README.rst b/tools/testing/selftests/bpf/README.rst
index 9b17f2867488..314a63b69399 100644
--- a/tools/testing/selftests/bpf/README.rst
+++ b/tools/testing/selftests/bpf/README.rst
@@ -228,3 +228,41 @@  To fix this issue, user newer libbpf.
 .. Links
 .. _clang reloc patch: https://reviews.llvm.org/D102712
 .. _kernel llvm reloc: /Documentation/bpf/llvm_reloc.rst
+
+bpf_iter_unix.o test failure and LLVM optimisation
+==================================================
+
+The ``bpf_iter/unix`` subtest requires `the fix`__ to prevent the LLVM compiler
+from transforming the loop exit condition.
+
+Without the fix, the compiler generates the following code:
+
+.. code-block:: c
+
+  ; 				 for (i = 1; i < len; i++)
+       110:	07 09 00 00 01 00 00 00	r9 += 1
+       111:	5d 98 09 00 00 00 00 00	if r8 != r9 goto +9 <LBB0_18>
+
+The loop exit condition, where the upper bound is not a constant, is
+changed from ``<`` to ``!=``.  Thus, the verifier always evaluates it as true,
+misleading to an infinite loop.
+
+.. code-block:: c
+
+  The sequence of 8193 jumps is too complex.
+  processed 196506 insns (limit 1000000) max_states_per_insn 4 total_states 1830 peak_states 1830 mark_read 3
+
+The fix prevents the optimisation by estimating its cost higher in such a
+case:
+
+.. code-block:: c
+
+  ; 				 for (i = 1; i < len; i++)
+       110:	07 09 00 00 01 00 00 00	r9 += 1
+       111:	ad 89 09 00 00 00 00 00	if r9 < r8 goto +9 <LBB0_18>
+
+The patch is available in LLVM 14 trunk and has been `backported`__ to the LLVM
+13.x release branch.
+
+__ https://reviews.llvm.org/D107483
+__ https://bugs.llvm.org/show_bug.cgi?id=51363
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
index 1f1aade56504..77ac24b191d4 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
@@ -13,6 +13,7 @@ 
 #include "bpf_iter_tcp6.skel.h"
 #include "bpf_iter_udp4.skel.h"
 #include "bpf_iter_udp6.skel.h"
+#include "bpf_iter_unix.skel.h"
 #include "bpf_iter_test_kern1.skel.h"
 #include "bpf_iter_test_kern2.skel.h"
 #include "bpf_iter_test_kern3.skel.h"
@@ -313,6 +314,19 @@  static void test_udp6(void)
 	bpf_iter_udp6__destroy(skel);
 }
 
+static void test_unix(void)
+{
+	struct bpf_iter_unix *skel;
+
+	skel = bpf_iter_unix__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "bpf_iter_unix__open_and_load"))
+		return;
+
+	do_dummy_read(skel->progs.dump_unix);
+
+	bpf_iter_unix__destroy(skel);
+}
+
 /* The expected string is less than 16 bytes */
 static int do_read_with_fd(int iter_fd, const char *expected,
 			   bool read_one_char)
@@ -1255,6 +1269,8 @@  void test_bpf_iter(void)
 		test_udp4();
 	if (test__start_subtest("udp6"))
 		test_udp6();
+	if (test__start_subtest("unix"))
+		test_unix();
 	if (test__start_subtest("anon"))
 		test_anon_iter(false);
 	if (test__start_subtest("anon-read-one-char"))
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter.h b/tools/testing/selftests/bpf/progs/bpf_iter.h
index 3d83b185c4bc..8cfaeba1ddbf 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter.h
+++ b/tools/testing/selftests/bpf/progs/bpf_iter.h
@@ -12,6 +12,7 @@ 
 #define tcp6_sock tcp6_sock___not_used
 #define bpf_iter__udp bpf_iter__udp___not_used
 #define udp6_sock udp6_sock___not_used
+#define bpf_iter__unix bpf_iter__unix___not_used
 #define bpf_iter__bpf_map_elem bpf_iter__bpf_map_elem___not_used
 #define bpf_iter__bpf_sk_storage_map bpf_iter__bpf_sk_storage_map___not_used
 #define bpf_iter__sockmap bpf_iter__sockmap___not_used
@@ -32,6 +33,7 @@ 
 #undef tcp6_sock
 #undef bpf_iter__udp
 #undef udp6_sock
+#undef bpf_iter__unix
 #undef bpf_iter__bpf_map_elem
 #undef bpf_iter__bpf_sk_storage_map
 #undef bpf_iter__sockmap
@@ -103,6 +105,12 @@  struct udp6_sock {
 	struct ipv6_pinfo inet6;
 } __attribute__((preserve_access_index));
 
+struct bpf_iter__unix {
+	struct bpf_iter_meta *meta;
+	struct unix_sock *unix_sk;
+	uid_t uid;
+} __attribute__((preserve_access_index));
+
 struct bpf_iter__bpf_map_elem {
 	struct bpf_iter_meta *meta;
 	struct bpf_map *map;
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_unix.c b/tools/testing/selftests/bpf/progs/bpf_iter_unix.c
new file mode 100644
index 000000000000..0a50b610f6f3
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_unix.c
@@ -0,0 +1,77 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright Amazon.com Inc. or its affiliates. */
+#include "bpf_iter.h"
+#include "bpf_tracing_net.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+
+char _license[] SEC("license") = "GPL";
+
+static long sock_i_ino(const struct sock *sk)
+{
+	const struct socket *sk_socket = sk->sk_socket;
+	const struct inode *inode;
+	unsigned long ino;
+
+	if (!sk_socket)
+		return 0;
+
+	inode = &container_of(sk_socket, struct socket_alloc, socket)->vfs_inode;
+	bpf_probe_read_kernel(&ino, sizeof(ino), &inode->i_ino);
+	return ino;
+}
+
+SEC("iter/unix")
+int dump_unix(struct bpf_iter__unix *ctx)
+{
+	struct unix_sock *unix_sk = ctx->unix_sk;
+	struct sock *sk = (struct sock *)unix_sk;
+	struct seq_file *seq;
+	__u32 seq_num;
+
+	if (!unix_sk)
+		return 0;
+
+	seq = ctx->meta->seq;
+	seq_num = ctx->meta->seq_num;
+	if (seq_num == 0)
+		BPF_SEQ_PRINTF(seq, "Num               RefCount Protocol Flags    Type St Inode    Path\n");
+
+	BPF_SEQ_PRINTF(seq, "%pK: %08X %08X %08X %04X %02X %8lu",
+		       unix_sk,
+		       sk->sk_refcnt.refs.counter,
+		       0,
+		       sk->sk_state == TCP_LISTEN ? __SO_ACCEPTCON : 0,
+		       sk->sk_type,
+		       sk->sk_socket ?
+		       (sk->sk_state == TCP_ESTABLISHED ? SS_CONNECTED : SS_UNCONNECTED) :
+		       (sk->sk_state == TCP_ESTABLISHED ? SS_CONNECTING : SS_DISCONNECTING),
+		       sock_i_ino(sk));
+
+	if (unix_sk->addr) {
+		if (!UNIX_ABSTRACT(unix_sk)) {
+			BPF_SEQ_PRINTF(seq, " %s", unix_sk->addr->name->sun_path);
+		} else {
+			/* The name of the abstract UNIX domain socket starts
+			 * with '\0' and can contain '\0'.  The null bytes
+			 * should be escaped as done in unix_seq_show().
+			 */
+			int i, len;
+
+			len = unix_sk->addr->len - sizeof(short);
+
+			BPF_SEQ_PRINTF(seq, " @");
+
+			/* unix_mkname() tests this upper bound. */
+			if (len < sizeof(struct sockaddr_un))
+				for (i = 1; i < len; i++)
+					BPF_SEQ_PRINTF(seq, "%c",
+						       unix_sk->addr->name->sun_path[i] ?:
+						       '@');
+		}
+	}
+
+	BPF_SEQ_PRINTF(seq, "\n");
+
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
index 3af0998a0623..eef5646ddb19 100644
--- a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
+++ b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
@@ -5,6 +5,10 @@ 
 #define AF_INET			2
 #define AF_INET6		10
 
+#define __SO_ACCEPTCON		(1 << 16)
+#define UNIX_HASH_SIZE		256
+#define UNIX_ABSTRACT(unix_sk)	(unix_sk->addr->hash < UNIX_HASH_SIZE)
+
 #define SOL_TCP			6
 #define TCP_CONGESTION		13
 #define TCP_CA_NAME_MAX		16