diff mbox series

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

Message ID 20240503113456.864063-2-aojea@google.com
State Changes Requested, archived
Headers show
Series netfilter: nfqueue: incorrect sctp checksum | expand

Commit Message

Antonio Ojea May 3, 2024, 11:34 a.m. UTC 
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(+)

Comments

Antonio Ojea May 3, 2024, 12:46 p.m. UTC | #1 
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_checksum_help(entskb))
+                   ((skb_csum_is_sctp(entskb) &&
skb_crc32c_csum_help(entskb)) ||
+                   skb_checksum_help(entskb)))

                data_len = READ_ONCE(queue->copy_range);

>                     skb_checksum_help(entskb))
>                         return NULL;
>
> --
> 2.45.0.rc1.225.g2a3ae87e7f-goog
>
Antonio Ojea May 3, 2024, 1:09 p.m. UTC | #2 
On Fri, May 3, 2024 at 2:46 PM Antonio Ojea <aojea@google.com> 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_checksum_help(entskb))
> +                   ((skb_csum_is_sctp(entskb) &&
> skb_crc32c_csum_help(entskb)) ||
> +                   skb_checksum_help(entskb)))
>

... and with this patch the regression test fails, so back to square 0.
It seems I still didn't find the root cause

>                 data_len = READ_ONCE(queue->copy_range);
>
> >                     skb_checksum_help(entskb))
> >                         return NULL;
> >
> > --
> > 2.45.0.rc1.225.g2a3ae87e7f-goog
> >
kernel test robot May 4, 2024, 1:51 a.m. UTC | #3 
Hi Antonio,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Antonio-Ojea/netfilter-nft_queue-compute-SCTP-checksum/20240503-193738
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240503113456.864063-2-aojea%40google.com
patch subject: [PATCH net-next 1/2] netfilter: nft_queue: compute SCTP checksum
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20240504/202405040912.TMSbJ9JW-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240504/202405040912.TMSbJ9JW-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405040912.TMSbJ9JW-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/dma/ioat/ioatdma.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/virtio/virtio_dma_buf.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/xen/xen-evtchn.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/xen/xen-privcmd.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/tty/n_hdlc.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/tty/n_gsm.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/char/agp/intel-gtt.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/char/lp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/char/ppdev.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/char/tlclk.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/gpu/drm/tiny/bochs.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/gpu/drm/tiny/cirrus.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/gpu/drm/i915/kvmgt.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/base/regmap/regmap-i2c.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/base/regmap/regmap-spi.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/block/brd.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/block/loop.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/block/null_blk/null_blk.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/dax/hmem/dax_hmem.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/dax/device_dax.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/dax/kmem.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/dax/dax_pmem.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/scsi/isci/isci.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/cdrom/cdrom.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/serial/usb_debug.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/media/tuners/tda9887.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/media/rc/rc-core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/media/dvb-frontends/au8522_decoder.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/media/dvb-frontends/mb86a16.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/media/v4l2-core/v4l2-async.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/media/v4l2-core/v4l2-fwnode.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hwmon/asus_atk0110.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/thermal/intel/intel_soc_dts_iosf.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/thermal/intel/int340x_thermal/processor_thermal_rapl.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/thermal/intel/int340x_thermal/processor_thermal_mbox.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/thermal/intel/int340x_thermal/processor_thermal_wt_req.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/thermal/intel/int340x_thermal/processor_thermal_wt_hint.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/thermal/intel/int340x_thermal/processor_thermal_power_floor.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mmc/core/mmc_core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mmc/core/sdio_uart.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-a4tech.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-apple.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-aureal.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-belkin.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-cherry.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-chicony.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-cypress.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-dr.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-elecom.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-ezkey.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-gyration.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-ite.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-kensington.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-keytouch.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-kye.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-lcpower.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-lenovo.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-logitech.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-lg-g15.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-logitech-dj.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-logitech-hidpp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-microsoft.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-monterey.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-ortek.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-pl.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-petalynx.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-primax.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-saitek.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-samsung.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-sjoy.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-speedlink.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-steelseries.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-sunplus.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-gaff.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-tmff.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-tivo.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-topseed.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-twinhan.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-xinmo.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-zpff.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-zydacron.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-waltop.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/intel-ish-hid/intel-ishtp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/platform/x86/intel/intel-hid.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/platform/x86/intel/intel-vbtn.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/platform/x86/intel/intel-rst.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/platform/x86/amilo-rfkill.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/platform/x86/classmate-laptop.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/ras/amd/atl/amd_atl.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hwtracing/intel_th/intel_th_msu_sink.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/parport/parport.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/nvdimm/libnvdimm.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/nvdimm/nd_pmem.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/nvdimm/nd_btt.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/nvdimm/nd_e820.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio_cif.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio_aec.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/dca/dca.o
>> ERROR: modpost: "skb_crc32c_csum_help" [net/netfilter/nfnetlink_queue.ko] undefined!
Pablo Neira Ayuso May 6, 2024, 10:30 p.m. UTC | #4 
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.
diff mbox series

Patch

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)) &&
 		    skb_checksum_help(entskb))
 			return NULL;