netdev - Re: [PATCH net-next v2 04/14] net: ipv4: Add ip_mr

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: <871prn4eim.fsf@nvidia.com>
Date: Fri, 13 Jun 2025 21:31:59 +0200
From: Petr Machata <petrm@...dia.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: Petr Machata <petrm@...dia.com>, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, "David
 Ahern" <dsahern@...il.com>, <netdev@...r.kernel.org>, Simon Horman
	<horms@...nel.org>, Nikolay Aleksandrov <razor@...ckwall.org>, Ido Schimmel
	<idosch@...dia.com>, <mlxsw@...dia.com>, Roopa Prabhu <roopa@...dia.com>,
	Benjamin Poirier <bpoirier@...dia.com>
Subject: Re: [PATCH net-next v2 04/14] net: ipv4: Add ip_mr_output()


Jakub Kicinski <kuba@...nel.org> writes:

> On Thu, 12 Jun 2025 22:10:38 +0200 Petr Machata wrote:
>> +	/* Forward the frame */
>> +	if (c->mfc_origin == htonl(INADDR_ANY) &&
>> +	    c->mfc_mcastgrp == htonl(INADDR_ANY)) {
>> +		if (ip_hdr(skb)->ttl >
>> +				c->_c.mfc_un.res.ttls[c->_c.mfc_parent]) {
>
> weird indent?

I suppose one extra tab would have sufficed.

>> +			/* It's an (*,*) entry and the packet is not coming from
>> +			 * the upstream: forward the packet to the upstream
>> +			 * only.
>> +			 */
>> +			psend = c->_c.mfc_parent;
>> +			goto last_xmit;
>> +		}
>> +		goto dont_xmit;
>> +	}
>> +
>> +	for (ct = c->_c.mfc_un.res.maxvif - 1;
>> +	     ct >= c->_c.mfc_un.res.minvif; ct--) {
>> +		if (ip_hdr(skb)->ttl > c->_c.mfc_un.res.ttls[ct]) {
>
> I'd be tempted to invert condition, continue, save a level of indent.
> Presumably we expect TTL to actually be large enough so that'd also
> make the expected path not under the if ?

The TTL condition is expressed several times across the MR code, always
in this way. It is the happy path, yeah, but I kept it like this for
consistency reasons.

>> +			if (psend != -1) {
>> +				struct sk_buff *skb2 = skb_clone(skb,
>> +								 GFP_ATOMIC);
>> +
>> +				if (skb2)
>
> maybe this is some local custom in this code but:
>
> 				struct sk_buff *skb2;
>
> 				skb2 = skb_clone(skb, GFP_ATOMIC);
> 				if (skb2)
>
> same LoC, less ugly.

OK.

>
>> +					ipmr_queue_output_xmit(net, mrt,
>> +							       skb2, psend);