diff mbox series

[net-next,5/7] icmp: reflect tos through ip cookie rather than updating inet_sk

Message ID 20250206193521.2285488-6-willemdebruijn.kernel@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: deduplicate cookie logic | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 0 this patch: 1
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 2 maintainers not CCed: horms@kernel.org dsahern@kernel.org
netdev/build_clang fail Errors and warnings before: 2 this patch: 3
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 2 this patch: 3
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 17 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 1 this patch: 1
netdev/source_inline success Was 0 now: 0

Commit Message

Willem de Bruijn Feb. 6, 2025, 7:34 p.m. UTC 
From: Willem de Bruijn <willemb@google.com>

Do not modify socket fields if it can be avoided.

The current code predates the introduction of ip cookies in commit
aa6615814533 ("ipv4: processing ancillary IP_TOS or IP_TTL"). Now that
cookies exist and support tos, update that field directly.

Signed-off-by: Willem de Bruijn <willemb@google.com>

---

Tested with ping -Q 32 127.0.0.1 and tcpdump

The existing logic works because inet->tos is read if ipc.tos (and
with that cork->tos) is left unitialized:

  iph->tos = (cork->tos != -1) ? cork->tos : READ_ONCE(inet->tos);
---
 net/ipv4/icmp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Willem de Bruijn Feb. 7, 2025, 1:01 a.m. UTC | #1 
Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
> 
> Do not modify socket fields if it can be avoided.
> 
> The current code predates the introduction of ip cookies in commit
> aa6615814533 ("ipv4: processing ancillary IP_TOS or IP_TTL"). Now that
> cookies exist and support tos, update that field directly.
> 
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> 
> ---
> 
> Tested with ping -Q 32 127.0.0.1 and tcpdump
> 
> The existing logic works because inet->tos is read if ipc.tos (and
> with that cork->tos) is left unitialized:
> 
>   iph->tos = (cork->tos != -1) ? cork->tos : READ_ONCE(inet->tos);
> ---
>  net/ipv4/icmp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> index 094084b61bff..9c5e052a7802 100644
> --- a/net/ipv4/icmp.c
> +++ b/net/ipv4/icmp.c
> @@ -429,7 +429,7 @@ static void icmp_reply(struct icmp_bxm *icmp_param, struct sk_buff *skb)
>  	icmp_param->data.icmph.checksum = 0;
>  
>  	ipcm_init(&ipc);
> -	inet->tos = ip_hdr(skb)->tos;
> +	ipc.tos = ip_hdr(skb)->tos;
>  	ipc.sockc.mark = mark;
>  	daddr = ipc.addr = ip_hdr(skb)->saddr;
>  	saddr = fib_compute_spec_dst(skb);

local variable inet is no longer used, needs to be removed.

Will fix in v2.
kernel test robot Feb. 8, 2025, 10:40 a.m. UTC | #2 
Hi Willem,

kernel test robot noticed the following build warnings:

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

url:    https://github.com/intel-lab-lkp/linux/commits/Willem-de-Bruijn/tcp-only-initialize-sockcm-tsflags-field/20250207-033912
base:   net-next/main
patch link:    https://lore.kernel.org/r/20250206193521.2285488-6-willemdebruijn.kernel%40gmail.com
patch subject: [PATCH net-next 5/7] icmp: reflect tos through ip cookie rather than updating inet_sk
config: x86_64-buildonly-randconfig-002-20250207 (https://download.01.org/0day-ci/archive/20250208/202502081845.hsTDUryC-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250208/202502081845.hsTDUryC-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/202502081845.hsTDUryC-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> net/ipv4/icmp.c:408:20: warning: variable 'inet' set but not used [-Wunused-but-set-variable]
     408 |         struct inet_sock *inet;
         |                           ^
   1 warning generated.


vim +/inet +408 net/ipv4/icmp.c

^1da177e4c3f41 Linus Torvalds         2005-04-16  395  
^1da177e4c3f41 Linus Torvalds         2005-04-16  396  /*
^1da177e4c3f41 Linus Torvalds         2005-04-16  397   *	Driving logic for building and sending ICMP messages.
^1da177e4c3f41 Linus Torvalds         2005-04-16  398   */
^1da177e4c3f41 Linus Torvalds         2005-04-16  399  
^1da177e4c3f41 Linus Torvalds         2005-04-16  400  static void icmp_reply(struct icmp_bxm *icmp_param, struct sk_buff *skb)
^1da177e4c3f41 Linus Torvalds         2005-04-16  401  {
^1da177e4c3f41 Linus Torvalds         2005-04-16  402  	struct ipcm_cookie ipc;
511c3f92ad5b6d Eric Dumazet           2009-06-02  403  	struct rtable *rt = skb_rtable(skb);
d8d1f30b95a635 Changli Gao            2010-06-10  404  	struct net *net = dev_net(rt->dst.dev);
8c2bd38b95f75f Eric Dumazet           2024-08-29  405  	bool apply_ratelimit = false;
77968b78242ee2 David S. Miller        2011-05-08  406  	struct flowi4 fl4;
fdc0bde90a689b Denis V. Lunev         2008-08-23  407  	struct sock *sk;
fdc0bde90a689b Denis V. Lunev         2008-08-23 @408  	struct inet_sock *inet;
35ebf65e851c6d David S. Miller        2012-06-28  409  	__be32 daddr, saddr;
e110861f86094c Lorenzo Colitti        2014-05-13  410  	u32 mark = IP4_REPLY_MARK(net, skb->mark);
c0303efeab7391 Jesper Dangaard Brouer 2017-01-09  411  	int type = icmp_param->data.icmph.type;
c0303efeab7391 Jesper Dangaard Brouer 2017-01-09  412  	int code = icmp_param->data.icmph.code;
^1da177e4c3f41 Linus Torvalds         2005-04-16  413  
91ed1e666a4ea2 Paolo Abeni            2017-08-03  414  	if (ip_options_echo(net, &icmp_param->replyopts.opt.opt, skb))
f00c401b9b5f0a Horms                  2006-02-02  415  		return;
^1da177e4c3f41 Linus Torvalds         2005-04-16  416  
8c2bd38b95f75f Eric Dumazet           2024-08-29  417  	/* Needed by both icmpv4_global_allow and icmp_xmit_lock */
7ba91ecb16824f Jesper Dangaard Brouer 2017-01-09  418  	local_bh_disable();
^1da177e4c3f41 Linus Torvalds         2005-04-16  419  
8c2bd38b95f75f Eric Dumazet           2024-08-29  420  	/* is global icmp_msgs_per_sec exhausted ? */
8c2bd38b95f75f Eric Dumazet           2024-08-29  421  	if (!icmpv4_global_allow(net, type, code, &apply_ratelimit))
7ba91ecb16824f Jesper Dangaard Brouer 2017-01-09  422  		goto out_bh_enable;
7ba91ecb16824f Jesper Dangaard Brouer 2017-01-09  423  
7ba91ecb16824f Jesper Dangaard Brouer 2017-01-09  424  	sk = icmp_xmit_lock(net);
7ba91ecb16824f Jesper Dangaard Brouer 2017-01-09  425  	if (!sk)
7ba91ecb16824f Jesper Dangaard Brouer 2017-01-09  426  		goto out_bh_enable;
7ba91ecb16824f Jesper Dangaard Brouer 2017-01-09  427  	inet = inet_sk(sk);
c0303efeab7391 Jesper Dangaard Brouer 2017-01-09  428  
^1da177e4c3f41 Linus Torvalds         2005-04-16  429  	icmp_param->data.icmph.checksum = 0;
^1da177e4c3f41 Linus Torvalds         2005-04-16  430  
351782067b6be8 Willem de Bruijn       2018-07-06  431  	ipcm_init(&ipc);
bbd17d3104f5a7 Willem de Bruijn       2025-02-06  432  	ipc.tos = ip_hdr(skb)->tos;
0da7536fb47f51 Willem de Bruijn       2020-07-01  433  	ipc.sockc.mark = mark;
9f6abb5f175bdb David S. Miller        2011-05-09  434  	daddr = ipc.addr = ip_hdr(skb)->saddr;
35ebf65e851c6d David S. Miller        2012-06-28  435  	saddr = fib_compute_spec_dst(skb);
aa6615814533c6 Francesco Fusco        2013-09-24  436  
f6d8bd051c391c Eric Dumazet           2011-04-21  437  	if (icmp_param->replyopts.opt.opt.optlen) {
f6d8bd051c391c Eric Dumazet           2011-04-21  438  		ipc.opt = &icmp_param->replyopts.opt;
f6d8bd051c391c Eric Dumazet           2011-04-21  439  		if (ipc.opt->opt.srr)
f6d8bd051c391c Eric Dumazet           2011-04-21  440  			daddr = icmp_param->replyopts.opt.opt.faddr;
^1da177e4c3f41 Linus Torvalds         2005-04-16  441  	}
77968b78242ee2 David S. Miller        2011-05-08  442  	memset(&fl4, 0, sizeof(fl4));
77968b78242ee2 David S. Miller        2011-05-08  443  	fl4.daddr = daddr;
35ebf65e851c6d David S. Miller        2012-06-28  444  	fl4.saddr = saddr;
e110861f86094c Lorenzo Colitti        2014-05-13  445  	fl4.flowi4_mark = mark;
e2d118a1cb5e60 Lorenzo Colitti        2016-11-04  446  	fl4.flowi4_uid = sock_net_uid(net, NULL);
0ed373390c5c18 Guillaume Nault        2024-10-22  447  	fl4.flowi4_tos = inet_dscp_to_dsfield(ip4h_dscp(ip_hdr(skb)));
77968b78242ee2 David S. Miller        2011-05-08  448  	fl4.flowi4_proto = IPPROTO_ICMP;
385add906b6155 David Ahern            2015-09-29  449  	fl4.flowi4_oif = l3mdev_master_ifindex(skb->dev);
3df98d79215ace Paul Moore             2020-09-27  450  	security_skb_classify_flow(skb, flowi4_to_flowi_common(&fl4));
9d6ec938019c6b David S. Miller        2011-03-12  451  	rt = ip_route_output_key(net, &fl4);
b23dd4fe42b455 David S. Miller        2011-03-02  452  	if (IS_ERR(rt))
^1da177e4c3f41 Linus Torvalds         2005-04-16  453  		goto out_unlock;
8c2bd38b95f75f Eric Dumazet           2024-08-29  454  	if (icmpv4_xrlim_allow(net, rt, &fl4, type, code, apply_ratelimit))
a15c89c703d434 Eric Dumazet           2022-01-24  455  		icmp_push_reply(sk, icmp_param, &fl4, &ipc, &rt);
^1da177e4c3f41 Linus Torvalds         2005-04-16  456  	ip_rt_put(rt);
^1da177e4c3f41 Linus Torvalds         2005-04-16  457  out_unlock:
405666db84b984 Denis V. Lunev         2008-02-29  458  	icmp_xmit_unlock(sk);
7ba91ecb16824f Jesper Dangaard Brouer 2017-01-09  459  out_bh_enable:
7ba91ecb16824f Jesper Dangaard Brouer 2017-01-09  460  	local_bh_enable();
^1da177e4c3f41 Linus Torvalds         2005-04-16  461  }
^1da177e4c3f41 Linus Torvalds         2005-04-16  462
diff mbox series

Patch

diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 094084b61bff..9c5e052a7802 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -429,7 +429,7 @@  static void icmp_reply(struct icmp_bxm *icmp_param, struct sk_buff *skb)
 	icmp_param->data.icmph.checksum = 0;
 
 	ipcm_init(&ipc);
-	inet->tos = ip_hdr(skb)->tos;
+	ipc.tos = ip_hdr(skb)->tos;
 	ipc.sockc.mark = mark;
 	daddr = ipc.addr = ip_hdr(skb)->saddr;
 	saddr = fib_compute_spec_dst(skb);
@@ -735,8 +735,8 @@  void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
 	icmp_param.data.icmph.checksum	 = 0;
 	icmp_param.skb	  = skb_in;
 	icmp_param.offset = skb_network_offset(skb_in);
-	inet_sk(sk)->tos = tos;
 	ipcm_init(&ipc);
+	ipc.tos = tos;
 	ipc.addr = iph->saddr;
 	ipc.opt = &icmp_param.replyopts.opt;
 	ipc.sockc.mark = mark;