From: Maxim Mikityanskiy <maxtram95@gmail.com>
To: Eduard Zingerman <eddyz87@gmail.com>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Andrii Nakryiko <andrii@kernel.org>,
John Fastabend <john.fastabend@gmail.com>,
Martin KaFai Lau <martin.lau@linux.dev>,
Song Liu <song@kernel.org>,
Yonghong Song <yonghong.song@linux.dev>,
KP Singh <kpsingh@kernel.org>,
Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>,
Jiri Olsa <jolsa@kernel.org>, Mykola Lysenko <mykolal@fb.com>,
Shuah Khan <shuah@kernel.org>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
Jesper Dangaard Brouer <hawk@kernel.org>,
linux-kselftest@vger.kernel.org, netdev@vger.kernel.org,
Maxim Mikityanskiy <maxim@isovalent.com>
Subject: Re: [PATCH bpf-next 12/15] bpf: Preserve boundaries and track scalars on narrowing fill
Date: Fri, 5 Jan 2024 19:48:25 +0200 [thread overview]
Message-ID: <ZZhA3LBjXEuCQH5Q@mail.gmail.com> (raw)
In-Reply-To: <a4c8b7b9f03ff3455fbf430862b370abe9337bc9.camel@gmail.com>
On Thu, 04 Jan 2024 at 04:27:00 +0200, Eduard Zingerman wrote:
> On Wed, 2023-12-20 at 23:40 +0200, Maxim Mikityanskiy wrote:
>
> [...]
>
> The two tests below were added by the following commit:
> ef979017b837 ("bpf: selftest: Add verifier tests for <8-byte scalar spill and refill")
>
> As far as I understand, the original intent was to check the behavior
> for stack read/write with non-matching size.
> I think these tests are redundant after patch #13. Wdyt?
_6_offset_to_skb_data is for sure not redundant. I don't test a partial
fill from the most significant bits in my patch 13.
u16_offset_to_skb_data is somewhat similar to
fill_32bit_after_spill_64bit, but they aren't exactly the same: the
former spills (u32)20 and fills (u16)20 (the same value), while my test
spills (u64)0xXXXXXXXX00000000 and fills (u32)0 (the most significant
bits are stripped). Maybe u16_offset_to_skb_data is redundant, but more
coverage is better than less coverage, isn't it?
> > diff --git a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> > index 809a09732168..de03e72e07a9 100644
> > --- a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> > +++ b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> > @@ -217,7 +217,7 @@ __naked void uninit_u32_from_the_stack(void)
> >
> > SEC("tc")
> > __description("Spill a u32 const scalar. Refill as u16. Offset to skb->data")
> > -__failure __msg("invalid access to packet")
> > +__success __retval(0)
> > __naked void u16_offset_to_skb_data(void)
> > {
> > asm volatile (" \
> > @@ -225,19 +225,24 @@ __naked void u16_offset_to_skb_data(void)
> > r3 = *(u32*)(r1 + %[__sk_buff_data_end]); \
> > w4 = 20; \
> > *(u32*)(r10 - 8) = r4; \
> > - r4 = *(u16*)(r10 - 8); \
> > + r4 = *(u16*)(r10 - %[offset]); \
> > r0 = r2; \
> > - /* r0 += r4 R0=pkt R2=pkt R3=pkt_end R4=umax=65535 */\
> > + /* r0 += r4 R0=pkt R2=pkt R3=pkt_end R4=20 */\
> > r0 += r4; \
> > - /* if (r0 > r3) R0=pkt,umax=65535 R2=pkt R3=pkt_end R4=umax=65535 */\
> > + /* if (r0 > r3) R0=pkt,off=20 R2=pkt R3=pkt_end R4=20 */\
> > if r0 > r3 goto l0_%=; \
> > - /* r0 = *(u32 *)r2 R0=pkt,umax=65535 R2=pkt R3=pkt_end R4=20 */\
> > + /* r0 = *(u32 *)r2 R0=pkt,off=20 R2=pkt R3=pkt_end R4=20 */\
> > r0 = *(u32*)(r2 + 0); \
> > l0_%=: r0 = 0; \
> > exit; \
> > " :
> > : __imm_const(__sk_buff_data, offsetof(struct __sk_buff, data)),
> > - __imm_const(__sk_buff_data_end, offsetof(struct __sk_buff, data_end))
> > + __imm_const(__sk_buff_data_end, offsetof(struct __sk_buff, data_end)),
> > +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> > + __imm_const(offset, 8)
> > +#else
> > + __imm_const(offset, 6)
> > +#endif
> > : __clobber_all);
> > }
> >
> > @@ -270,7 +275,7 @@ l0_%=: r0 = 0; \
> > }
> >
> > SEC("tc")
> > -__description("Spill a u32 const scalar. Refill as u16 from fp-6. Offset to skb->data")
> > +__description("Spill a u32 const scalar. Refill as u16 from MSB. Offset to skb->data")
> > __failure __msg("invalid access to packet")
> > __naked void _6_offset_to_skb_data(void)
> > {
> > @@ -279,7 +284,7 @@ __naked void _6_offset_to_skb_data(void)
> > r3 = *(u32*)(r1 + %[__sk_buff_data_end]); \
> > w4 = 20; \
> > *(u32*)(r10 - 8) = r4; \
> > - r4 = *(u16*)(r10 - 6); \
> > + r4 = *(u16*)(r10 - %[offset]); \
> > r0 = r2; \
> > /* r0 += r4 R0=pkt R2=pkt R3=pkt_end R4=umax=65535 */\
> > r0 += r4; \
> > @@ -291,7 +296,12 @@ l0_%=: r0 = 0; \
> > exit; \
> > " :
> > : __imm_const(__sk_buff_data, offsetof(struct __sk_buff, data)),
> > - __imm_const(__sk_buff_data_end, offsetof(struct __sk_buff, data_end))
> > + __imm_const(__sk_buff_data_end, offsetof(struct __sk_buff, data_end)),
> > +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> > + __imm_const(offset, 6)
> > +#else
> > + __imm_const(offset, 8)
> > +#endif
> > : __clobber_all);
> > }
> >
>
>
>
next prev parent reply other threads:[~2024-01-05 17:48 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-20 21:39 [PATCH bpf-next 00/15] Improvements for tracking scalars in the BPF verifier Maxim Mikityanskiy
2023-12-20 21:39 ` [PATCH bpf-next 01/15] selftests/bpf: Fix the u64_offset_to_skb_data test Maxim Mikityanskiy
2023-12-26 9:52 ` Shung-Hsi Yu
2023-12-26 10:38 ` Maxim Mikityanskiy
2023-12-26 13:22 ` Shung-Hsi Yu
2024-01-04 2:22 ` Eduard Zingerman
2023-12-20 21:40 ` [PATCH bpf-next 02/15] bpf: make infinite loop detection in is_state_visited() exact Maxim Mikityanskiy
2023-12-20 21:40 ` [PATCH bpf-next 03/15] selftests/bpf: check if imprecise stack spills confuse infinite loop detection Maxim Mikityanskiy
2023-12-20 21:40 ` [PATCH bpf-next 04/15] bpf: Make bpf_for_each_spilled_reg consider narrow spills Maxim Mikityanskiy
2024-01-04 2:25 ` Eduard Zingerman
2023-12-20 21:40 ` [PATCH bpf-next 05/15] selftests/bpf: Add a test case for 32-bit spill tracking Maxim Mikityanskiy
2024-01-04 2:25 ` Eduard Zingerman
2023-12-20 21:40 ` [PATCH bpf-next 06/15] bpf: Add the assign_scalar_id_before_mov function Maxim Mikityanskiy
2024-01-04 2:26 ` Eduard Zingerman
2023-12-20 21:40 ` [PATCH bpf-next 07/15] bpf: Add the get_reg_width function Maxim Mikityanskiy
2023-12-20 21:40 ` [PATCH bpf-next 08/15] bpf: Assign ID to scalars on spill Maxim Mikityanskiy
2023-12-25 3:15 ` Alexei Starovoitov
2023-12-25 21:11 ` Maxim Mikityanskiy
2023-12-25 21:26 ` Alexei Starovoitov
2024-01-04 2:26 ` Eduard Zingerman
2023-12-20 21:40 ` [PATCH bpf-next 09/15] selftests/bpf: Test assigning " Maxim Mikityanskiy
2024-01-04 2:26 ` Eduard Zingerman
2023-12-20 21:40 ` [PATCH bpf-next 10/15] bpf: Track spilled unbounded scalars Maxim Mikityanskiy
2024-01-04 2:26 ` Eduard Zingerman
2023-12-20 21:40 ` [PATCH bpf-next 11/15] selftests/bpf: Test tracking " Maxim Mikityanskiy
2024-01-04 2:26 ` Eduard Zingerman
2023-12-20 21:40 ` [PATCH bpf-next 12/15] bpf: Preserve boundaries and track scalars on narrowing fill Maxim Mikityanskiy
2023-12-26 5:29 ` Shung-Hsi Yu
2024-01-04 2:27 ` Eduard Zingerman
2024-01-05 17:48 ` Maxim Mikityanskiy [this message]
2023-12-20 21:40 ` [PATCH bpf-next 13/15] selftests/bpf: Add test cases for " Maxim Mikityanskiy
2024-01-04 2:26 ` Eduard Zingerman
2023-12-20 21:40 ` [PATCH bpf-next 14/15] bpf: Optimize state pruning for spilled scalars Maxim Mikityanskiy
2023-12-20 21:40 ` [PATCH bpf-next 15/15] selftests/bpf: states pruning checks for scalar vs STACK_{MISC,ZERO} Maxim Mikityanskiy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZZhA3LBjXEuCQH5Q@mail.gmail.com \
--to=maxtram95@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=eddyz87@gmail.com \
--cc=haoluo@google.com \
--cc=hawk@kernel.org \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kpsingh@kernel.org \
--cc=kuba@kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=martin.lau@linux.dev \
--cc=maxim@isovalent.com \
--cc=mykolal@fb.com \
--cc=netdev@vger.kernel.org \
--cc=sdf@google.com \
--cc=shuah@kernel.org \
--cc=song@kernel.org \
--cc=yonghong.song@linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.