netdev - Re: [PATCH ipsec-next v8 10/16] xfrm: iptfs: add fragmenting of larger than MTU user packets

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <m2cymnpphe.fsf@ja-home.int.chopps.org>
Date: Mon, 05 Aug 2024 00:19:02 -0400
From: Christian Hopps <chopps@...pps.org>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: Christian Hopps <chopps@...pps.org>, devel@...ux-ipsec.org, Steffen
  Klassert <steffen.klassert@...unet.com>, netdev@...r.kernel.org,
 Christian  Hopps <chopps@...n.net>
Subject: Re: [PATCH ipsec-next v8 10/16] xfrm: iptfs: add fragmenting of
 larger than MTU user packets


Christian Hopps <chopps@...pps.org> writes:

> Sabrina Dubroca <sd@...asysnail.net> writes:

>>> +	/* The opportunity for HW offload has ended */
>>> +	if (skb->ip_summed == CHECKSUM_PARTIAL) {
>>> +		err = skb_checksum_help(skb);
>>> +		if (err)
>>> +			return err;
>>> +	}
>>> +
>>> +	/* We've split these up before queuing */
>>> +	BUG_ON(skb_is_gso(skb));
>>
>> As I've said previously, I don't think that's a valid reason to
>> crash. BUG_ON should be used very rarely:
>>
>> https://elixir.bootlin.com/linux/v6.10/source/Documentation/process/coding-style.rst#L1230
>>
>> Dropping a bogus packet is an easy way to recover from this situation,
>> so we should not crash here (and probably in all of IPTFS).
>
> This is basically following a style of coding that aims to simplify overall code
> by eliminating multiple checks for the same condition over and over in code. It
> does this by arranging for a single variant at the beginning of an operation and
> then counting on that from then on in the code. Asserts are the way to document
> this, if no assert then nothing b/c using a conditional is exactly against the
> design principle.
>
> An existing example of this in the kernel is `assert_spin_locked()`.
>
> Anyway, I will just remove it if this is going to block adoption of the patch.

Actually, I'll just convert all BUG_ON() to WARN_ON().

Thanks,
Chris.

> Thanks,
> Chris.