Re: [PATCH net-next 1/2] netfilter: nft_queue: compute SCTP checksum

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Fri, May 03, 2024 at 02:46:27PM +0200, Antonio Ojea wrote:
> On Fri, May 3, 2024 at 1:35 PM Antonio Ojea <aojea@google.com> wrote:
> >
> > when the packet is processed with GSO and is SCTP it has to take into
> > account the SCTP checksum.
> >
> > Signed-off-by: Antonio Ojea <aojea@google.com>
> > ---
> >  net/netfilter/nfnetlink_queue.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> > index 00f4bd21c59b..428014aea396 100644
> > --- a/net/netfilter/nfnetlink_queue.c
> > +++ b/net/netfilter/nfnetlink_queue.c
> > @@ -600,6 +600,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> >         case NFQNL_COPY_PACKET:
> >                 if (!(queue->flags & NFQA_CFG_F_GSO) &&
> >                     entskb->ip_summed == CHECKSUM_PARTIAL &&
> > +                   (skb_csum_is_sctp(entskb) && skb_crc32c_csum_help(entskb)) &&
> 
> My bad, this is wrong, it should be an OR so skb_checksum_help is
> always evaluated.
> Pablo suggested in the bugzilla to use a helper, so I'm not sure this
> is the right fix, I've tried
> to look for similar solutions to find a more consistent solution but
> I'm completely new to the
> kernel codebase so some guidance will be appreciated.

skb_crc32c_csum_help() returns 0 on success.

> -                   skb_checksum_help(entskb))
> +                   ((skb_csum_is_sctp(entskb) &&
> skb_crc32c_csum_help(entskb)) ||
> +                   skb_checksum_help(entskb)))

skb_crc32c_csum_help() returns 0 on success, then this calls
skb_checksum_help() which is TCP/UDP specific, which corrupts the
packet.

I think you have to move this to a helper function like:

static int nf_queue_checksum_help(struct sk_buff *entskb)
{
        if (skb_csum_is_sctp(entskb))
                return skb_crc32c_csum_help(entskb);

        return skb_checksum_help(entskb);
}

I can see skb_crc32c_csum_help() could return -EINVAL, the fallback to
skb_checksum_help() is not wanted there in such case.

Thanks.