Re: [PATCH bpf-next v3 1/2] bpf: add CHECKSUM_COMPLETE to bpf test progs

[Vadim Fedorenko <vadim.fedorenko@xxxxxxxxx>]

On 05/06/2024 15:43, Daniel Borkmann wrote:
> On 6/5/24 11:42 AM, Vadim Fedorenko wrote:
> > On 27/05/2024 19:59, Vadim Fedorenko wrote:
> > > Add special flag to validate that TC BPF program properly updates
> > > checksum information in skb.
> > >
> > > Signed-off-by: Vadim Fedorenko <vadfed@xxxxxxxx>
> > > ---
> > >   include/uapi/linux/bpf.h       |  2 ++
> > >   net/bpf/test_run.c             | 18 +++++++++++++++++-
> > >   tools/include/uapi/linux/bpf.h |  2 ++
> > >   3 files changed, 21 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > index 90706a47f6ff..f7d458d88111 100644
> > > --- a/include/uapi/linux/bpf.h
> > > +++ b/include/uapi/linux/bpf.h
> > > @@ -1425,6 +1425,8 @@ enum {
> > >   #define BPF_F_TEST_RUN_ON_CPU    (1U << 0)
> > >   /* If set, XDP frames will be transmitted after processing */
> > >   #define BPF_F_TEST_XDP_LIVE_FRAMES    (1U << 1)
> > > +/* If set, apply CHECKSUM_COMPLETE to skb and validate the checksum */
> > > +#define BPF_F_TEST_SKB_CHECKSUM_COMPLETE    (1U << 2)
> > >   /* type for BPF_ENABLE_STATS */
> > >   enum bpf_stats_type {
> > > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> > > index f6aad4ed2ab2..4c21562ad526 100644
> > > --- a/net/bpf/test_run.c
> > > +++ b/net/bpf/test_run.c
> > > @@ -977,7 +977,8 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, 
> > > const union bpf_attr *kattr,
> > >       void *data;
> > >       int ret;
> > > -    if (kattr->test.flags || kattr->test.cpu || kattr->test.batch_size)
> > > +    if ((kattr->test.flags & ~BPF_F_TEST_SKB_CHECKSUM_COMPLETE) ||
> > > +        kattr->test.cpu || kattr->test.batch_size)
> > >           return -EINVAL;
> > >       data = bpf_test_init(kattr, kattr->test.data_size_in,
> > > @@ -1025,6 +1026,12 @@ int bpf_prog_test_run_skb(struct bpf_prog 
> > > *prog, const union bpf_attr *kattr,
> > >       skb_reserve(skb, NET_SKB_PAD + NET_IP_ALIGN);
> > >       __skb_put(skb, size);
> > > +
> > > +    if (kattr->test.flags & BPF_F_TEST_SKB_CHECKSUM_COMPLETE) {
> > > +        skb->csum = skb_checksum(skb, 0, skb->len, 0);
> > > +        skb->ip_summed = CHECKSUM_COMPLETE;
> > > +    }
> > > +
> > >       if (ctx && ctx->ifindex > 1) {
> > >           dev = dev_get_by_index(net, ctx->ifindex);
> > >           if (!dev) {
> > > @@ -1079,6 +1086,15 @@ int bpf_prog_test_run_skb(struct bpf_prog 
> > > *prog, const union bpf_attr *kattr,
> > >       }
> > >       convert_skb_to___skb(skb, ctx);
> > > +    if (kattr->test.flags & BPF_F_TEST_SKB_CHECKSUM_COMPLETE) {
> > > +        __wsum csum = skb_checksum(skb, 0, skb->len, 0);
> > > +
> > > +        if (skb->csum != csum) {
> > > +            ret = -EBADMSG;
> > > +            goto out;
> > > +        }
> > > +    }
> > > +
> > >       size = skb->len;
> > >       /* bpf program can never convert linear skb to non-linear */
> > >       if (WARN_ON_ONCE(skb_is_nonlinear(skb)))
> > > diff --git a/tools/include/uapi/linux/bpf.h 
> > > b/tools/include/uapi/linux/bpf.h
> > > index 90706a47f6ff..f7d458d88111 100644
> > > --- a/tools/include/uapi/linux/bpf.h
> > > +++ b/tools/include/uapi/linux/bpf.h
> > > @@ -1425,6 +1425,8 @@ enum {
> > >   #define BPF_F_TEST_RUN_ON_CPU    (1U << 0)
> > >   /* If set, XDP frames will be transmitted after processing */
> > >   #define BPF_F_TEST_XDP_LIVE_FRAMES    (1U << 1)
> > > +/* If set, apply CHECKSUM_COMPLETE to skb and validate the checksum */
> > > +#define BPF_F_TEST_SKB_CHECKSUM_COMPLETE    (1U << 2)
> > >   /* type for BPF_ENABLE_STATS */
> > >   enum bpf_stats_type {
> >
> > Hi Daniel!
> >
> > Have you had a chance to look at v3 of this patch?
> > I think I addressed all your comments form v2.
>
> Looks good, but I think there is something off given the test on arm64 
> and s390x
> fails in skb_pkt_end with EBADMSG. I wonder if we're missing a :
>
>    skb_postpull_rcsum(skb, eth_hdr(skb), ETH_HLEN);
>
> right after the eth_type_trans()?

Oh, thanks for bringing it up. Looks like it's a bit more complex.
Apparently, CHECKSUM_COMPLETE covers everything after the ethernet
header. That's why the code has to calculate checksum after network and
transport offsets are set. And for L2 case we have to skip ethernet
header.

Another issue is that correct check should use folded checksums instead
of raw values. I believe that was flagged by tests for other archs.

I'll do v4 soon and will be sure to have tests passing on all archs.

Thanks,
Vadim



> Thanks,
> Daniel