Received-SPF: pass (google.com: domain of linux-kernel+bounces-146568-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 16 Apr 2024 11:45:56 +0200
From: Joel Granados <j.granados@samsung.com>
To: Kuniyuki Iwashima <kuniyu@amazon.com>
CC: <devnull+j.granados.samsung.com@kernel.org>, <Dai.Ngo@oracle.com>,
	<alex.aring@gmail.com>, <alibuda@linux.alibaba.com>,
	<allison.henderson@oracle.com>, <anna@kernel.org>, <bridge@lists.linux.dev>,
	<chuck.lever@oracle.com>, <coreteam@netfilter.org>, <courmisch@gmail.com>,
	<davem@davemloft.net>, <dccp@vger.kernel.org>, <dhowells@redhat.com>,
	<dsahern@kernel.org>, <edumazet@google.com>, <fw@strlen.de>,
	<geliang@kernel.org>, <guwen@linux.alibaba.com>,
	<herbert@gondor.apana.org.au>, <horms@verge.net.au>, <ja@ssi.bg>,
	<jaka@linux.ibm.com>, <jlayton@kernel.org>, <jmaloy@redhat.com>,
	<jreuter@yaina.de>, <kadlec@netfilter.org>, <keescook@chromium.org>,
	<kolga@netapp.com>, <kuba@kernel.org>, <linux-afs@lists.infradead.org>,
	<linux-hams@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-nfs@vger.kernel.org>, <linux-rdma@vger.kernel.org>,
	<linux-s390@vger.kernel.org>, <linux-sctp@vger.kernel.org>,
	<linux-wpan@vger.kernel.org>, <linux-x25@vger.kernel.org>,
	<lucien.xin@gmail.com>, <lvs-devel@vger.kernel.org>,
	<marc.dionne@auristor.com>, <marcelo.leitner@gmail.com>,
	<martineau@kernel.org>, <matttbe@kernel.org>, <mcgrof@kernel.org>,
	<miquel.raynal@bootlin.com>, <mptcp@lists.linux.dev>, <ms@dev.tdt.de>,
	<neilb@suse.de>, <netdev@vger.kernel.org>,
	<netfilter-devel@vger.kernel.org>, <pabeni@redhat.com>,
	<pablo@netfilter.org>, <ralf@linux-mips.org>, <razor@blackwall.org>,
	<rds-devel@oss.oracle.com>, <roopa@nvidia.com>, <stefan@datenfreihafen.org>,
	<steffen.klassert@secunet.com>, <tipc-discussion@lists.sourceforge.net>,
	<tom@talpey.com>, <tonylu@linux.alibaba.com>,
	<trond.myklebust@hammerspace.com>, <wenjia@linux.ibm.com>,
	<ying.xue@windriver.com>
Subject: Re: [PATCH v3 1/4] networking: Remove the now superfluous sentinel
 elements from ctl_table array
Message-ID: <20240416094556.cv4k3uqthqqpln3h@joelS2.panther.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg="pgp-sha512";
	protocol="application/pgp-signature"; boundary="ebocjdihejx32jpt"
Content-Disposition: inline
In-Reply-To: <20240415231210.22785-1-kuniyu@amazon.com>
CMS-TYPE: 201P
References: <20240412-jag-sysctl_remset_net-v3-1-11187d13c211@samsung.com>
	<CGME20240415231242eucas1p2c2b98fffb9b4752a3771f343d7a53dfe@eucas1p2.samsung.com>
	<20240415231210.22785-1-kuniyu@amazon.com>

--ebocjdihejx32jpt
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Apr 15, 2024 at 04:12:10PM -0700, Kuniyuki Iwashima wrote:
> From: Joel Granados via B4 Relay <devnull+j.granados.samsung.com@kernel.o=
rg>
> Date: Fri, 12 Apr 2024 16:48:29 +0200
> > From: Joel Granados <j.granados@samsung.com>
=2E..
> > Signed-off-by: Joel Granados <j.granados@samsung.com>
> > ---
> >  net/core/neighbour.c                | 5 +----
> >  net/core/sysctl_net_core.c          | 9 ++++-----
> >  net/dccp/sysctl.c                   | 2 --
> >  net/ieee802154/6lowpan/reassembly.c | 6 +-----
> >  net/ipv4/devinet.c                  | 5 ++---
> >  net/ipv4/ip_fragment.c              | 2 --
> >  net/ipv4/route.c                    | 8 ++------
> >  net/ipv4/sysctl_net_ipv4.c          | 7 +++----
> >  net/ipv4/xfrm4_policy.c             | 1 -
> >  net/ipv6/addrconf.c                 | 5 +----
> >  net/ipv6/icmp.c                     | 1 -
> >  net/ipv6/reassembly.c               | 2 --
> >  net/ipv6/route.c                    | 5 -----
> >  net/ipv6/sysctl_net_ipv6.c          | 4 +---
> >  net/ipv6/xfrm6_policy.c             | 1 -
> >  net/llc/sysctl_net_llc.c            | 8 ++------
> >  net/mpls/af_mpls.c                  | 3 +--
> >  net/mptcp/ctrl.c                    | 1 -
> >  net/netrom/sysctl_net_netrom.c      | 1 -
> >  net/phonet/sysctl.c                 | 1 -
> >  net/rds/ib_sysctl.c                 | 1 -
> >  net/rds/sysctl.c                    | 1 -
> >  net/rds/tcp.c                       | 1 -
> >  net/rose/sysctl_net_rose.c          | 1 -
> >  net/rxrpc/sysctl.c                  | 1 -
> >  net/sctp/sysctl.c                   | 6 +-----
> >  net/smc/smc_sysctl.c                | 1 -
> >  net/sunrpc/sysctl.c                 | 1 -
> >  net/sunrpc/xprtrdma/svc_rdma.c      | 1 -
> >  net/sunrpc/xprtrdma/transport.c     | 1 -
> >  net/sunrpc/xprtsock.c               | 1 -
> >  net/tipc/sysctl.c                   | 1 -
> >  net/unix/sysctl_net_unix.c          | 1 -
> >  net/x25/sysctl_net_x25.c            | 1 -
> >  net/xfrm/xfrm_sysctl.c              | 5 +----
> >  35 files changed, 20 insertions(+), 81 deletions(-)
>=20
> You may want to split patch based on subsystem or the type of changes
> to make review easier.

That is fair. It is a big chunk:). I'll put the trivial patches together
to avoid having an 18 commits to instead have 8. This is my proposal
based on MAINTAINERS file:

### Not in MAINTAINERS / Orphaned
net/core/neighbour.c
net/core/sysctl_net_core.c
net/ieee802154/6lowpan/reassembly.c
net/mpls/af_mpls.c
net/unix/sysctl_net_unix.c
net/dccp/sysctl.c

### NETWORKING
net/ipv4/devinet.c
net/ipv4/ip_fragment.c
net/ipv4/route.c
net/ipv4/sysctl_net_ipv4.c
net/ipv4/xfrm4_policy.c
net/ipv6/addrconf.c
net/ipv6/icmp.c
net/ipv6/reassembly.c
net/ipv6/route.c
net/ipv6/sysctl_net_ipv6.c
net/ipv6/xfrm6_policy.c

### RDS
net/rds/ib_sysctl.c
net/rds/sysctl.c
net/rds/tcp.c

### SUNRPC
net/sunrpc/sysctl.c
net/sunrpc/xprtrdma/svc_rdma.c
net/sunrpc/xprtrdma/transport.c
net/sunrpc/xprtsock.c

### LLC/MTCP/NETROM/PHONET/ROSE/RXRPC/SCTP/SMC/TIPC/x.25/IPSEC
net/llc/sysctl_net_llc.c
net/mptcp/ctrl.c
net/netrom/sysctl_net_netrom.c
net/phonet/sysctl.c
net/rose/sysctl_net_rose.c
net/rxrpc/sysctl.c
net/sctp/sysctl.c
net/smc/smc_sysctl.c
net/tipc/sysctl.c
net/x25/sysctl_net_x25.c
net/xfrm/xfrm_sysctl.c

>=20
>=20
> >=20
> > diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> > index 552719c3bbc3..b0327402b3e6 100644
> > --- a/net/core/neighbour.c
> > +++ b/net/core/neighbour.c
> > @@ -3728,7 +3728,7 @@ static int neigh_proc_base_reachable_time(struct =
ctl_table *ctl, int write,
> > =20
> >  static struct neigh_sysctl_table {
> >  	struct ctl_table_header *sysctl_header;
> > -	struct ctl_table neigh_vars[NEIGH_VAR_MAX + 1];
> > +	struct ctl_table neigh_vars[NEIGH_VAR_MAX];
> >  } neigh_sysctl_template __read_mostly =3D {
> >  	.neigh_vars =3D {
> >  		NEIGH_SYSCTL_ZERO_INTMAX_ENTRY(MCAST_PROBES, "mcast_solicit"),
> > @@ -3779,7 +3779,6 @@ static struct neigh_sysctl_table {
> >  			.extra2		=3D SYSCTL_INT_MAX,
> >  			.proc_handler	=3D proc_dointvec_minmax,
> >  		},
> > -		{},
> >  	},
> >  };
> > =20
> > @@ -3807,8 +3806,6 @@ int neigh_sysctl_register(struct net_device *dev,=
 struct neigh_parms *p,
> >  	if (dev) {
> >  		dev_name_source =3D dev->name;
> >  		/* Terminate the table early */
>=20
> You can remove this comment.
Why? I do not think we should remove it because it is what the change to
neigh_vars_size is doing.

>=20
>=20
> > -		memset(&t->neigh_vars[NEIGH_VAR_GC_INTERVAL], 0,
> > -		       sizeof(t->neigh_vars[NEIGH_VAR_GC_INTERVAL]));
> >  		neigh_vars_size =3D NEIGH_VAR_BASE_REACHABLE_TIME_MS + 1;
> >  	} else {
> >  		struct neigh_table *tbl =3D p->tbl;
> > diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
> > index 6973dda3abda..46f5143e86be 100644
> > --- a/net/core/sysctl_net_core.c
> > +++ b/net/core/sysctl_net_core.c
> > @@ -660,7 +660,6 @@ static struct ctl_table net_core_table[] =3D {
> >  		.proc_handler	=3D proc_dointvec_minmax,
> >  		.extra1		=3D SYSCTL_ZERO,
> >  	},
> > -	{ }
> >  };
> > =20
> >  static struct ctl_table netns_core_table[] =3D {
> > @@ -697,7 +696,6 @@ static struct ctl_table netns_core_table[] =3D {
> >  		.extra2		=3D SYSCTL_ONE,
> >  		.proc_handler	=3D proc_dou8vec_minmax,
> >  	},
> > -	{ }
> >  };
> > =20
> >  static int __init fb_tunnels_only_for_init_net_sysctl_setup(char *str)
> > @@ -715,7 +713,8 @@ __setup("fb_tunnels=3D", fb_tunnels_only_for_init_n=
et_sysctl_setup);
> > =20
> >  static __net_init int sysctl_core_net_init(struct net *net)
> >  {
> > -	struct ctl_table *tbl, *tmp;
> > +	struct ctl_table *tbl;
> > +	size_t table_size =3D ARRAY_SIZE(netns_core_table);
>=20
> When you add a new variable, please keep reverse xmas tree.
Thx for pointing this out. Was not aware of this quirk in net code. Will
include it for my next version.

>=20
> Also, you can reuse this variable for the following
> register_net_sysctl_sz(), but it's inconsistent in the
> this patch..
>=20
>   table_size
>   * sysctl_route_net_init
>   * ipv4_sysctl_init_net
>=20
>   ARRAY_SIZE
>   * __addrconf_sysctl_register
>   * ipv6_sysctl_net_init
>   * mpls_dev_sysctl_register
>   * sctp_sysctl_net_register
>=20
>=20
> > =20
> >  	tbl =3D netns_core_table;
> >  	if (!net_eq(net, &init_net)) {
> > @@ -723,8 +722,8 @@ static __net_init int sysctl_core_net_init(struct n=
et *net)
> >  		if (tbl =3D=3D NULL)
> >  			goto err_dup;
> > =20
> > -		for (tmp =3D tbl; tmp->procname; tmp++)
> > -			tmp->data +=3D (char *)net - (char *)&init_net;
> > +		for (int i =3D 0; i < table_size; ++i)
> >  	.devinet_vars =3D {
=2E..
> >  		.extra1		=3D SYSCTL_ONE,
> >  	},
> > -	{ }
> >  };
> > =20
> >  static __net_init int ipv4_sysctl_init_net(struct net *net)
> >  {
> >  	struct ctl_table *table;
> > +	size_t table_size =3D ARRAY_SIZE(ipv4_net_table);
>=20
> nit: keep reverse xmax tree order.
Ok.

>=20
>=20
> > =20
> >  	table =3D ipv4_net_table;
> >  	if (!net_eq(net, &init_net)) {
> > @@ -1517,7 +1516,7 @@ static __net_init int ipv4_sysctl_init_net(struct=
 net *net)
> >  		if (!table)
> >  			goto err_alloc;
> > =20
> > -		for (i =3D 0; i < ARRAY_SIZE(ipv4_net_table) - 1; i++) {
=2E..
> > =20
> >  static int __addrconf_sysctl_register(struct net *net, char *dev_name,
> > @@ -7197,7 +7194,7 @@ static int __addrconf_sysctl_register(struct net =
*net, char *dev_name,
> >  	if (!table)
> >  		goto out;
> > =20
> > -	for (i =3D 0; table[i].data; i++) {
> > +	for (i =3D 0; i < ARRAY_SIZE(addrconf_sysctl); i++) {
>=20
>                         ^^^
Did you mean reuse variable here? What does this mean?

>=20
>=20
> >  		table[i].data +=3D (char *)p - (char *)&ipv6_devconf;
> >  		/* If one of these is already set, then it is not safe to
> >  		 * overwrite either of them: this makes proc_dointvec_minmax
> > diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
> > index 1635da07285f..91cbf8e8009f 100644
> > --- a/net/ipv6/icmp.c
> > +++ b/net/ipv6/icmp.c
> > @@ -1206,7 +1206,6 @@ static struct ctl_table ipv6_icmp_table_template[=
] =3D {
> >  		.extra1		=3D SYSCTL_ZERO,
=2E..
> > -	{ }
> >  };
> > =20
> >  static int __net_init ipv6_sysctl_net_init(struct net *net)
> > @@ -264,7 +262,7 @@ static int __net_init ipv6_sysctl_net_init(struct n=
et *net)
> >  	if (!ipv6_table)
> >  		goto out;
> >  	/* Update the variables to point into the current struct net */
> > -	for (i =3D 0; i < ARRAY_SIZE(ipv6_table_template) - 1; i++)
> > +	for (i =3D 0; i < ARRAY_SIZE(ipv6_table_template); i++)
>=20
>                         ^^^
Did you mean reuse variable here? What does this mean?
>=20
>=20
> >  		ipv6_table[i].data +=3D (void *)net - (void *)&init_net;
> > =20
> >  	ipv6_route_table =3D ipv6_route_sysctl_init(net);
> > diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
> > index 42fb6996b077..499b5f5c19fc 100644
> > --- a/net/ipv6/xfrm6_policy.c
> > +++ b/net/ipv6/xfrm6_policy.c
> > @@ -184,7 +184,6 @@ static struct ctl_table xfrm6_policy_table[] =3D {
> >  		.mode		=3D 0644,
> >  		.proc_handler   =3D proc_dointvec,
> >  	},
> > -	{ }
> >  };
> > =20
> >  static int __net_init xfrm6_net_sysctl_init(struct net *net)
> > diff --git a/net/llc/sysctl_net_llc.c b/net/llc/sysctl_net_llc.c
> > index 8443a6d841b0..72e101135f8c 100644
> > --- a/net/llc/sysctl_net_llc.c
> > +++ b/net/llc/sysctl_net_llc.c
> > @@ -44,11 +44,6 @@ static struct ctl_table llc2_timeout_table[] =3D {
> >  		.mode		=3D 0644,
> >  		.proc_handler   =3D proc_dointvec_jiffies,
> >  	},
> > -	{ },
> > -};
> > -
> > -static struct ctl_table llc_station_table[] =3D {
> > -	{ },
> >  };
> > =20
> >  static struct ctl_table_header *llc2_timeout_header;
> > @@ -56,8 +51,9 @@ static struct ctl_table_header *llc_station_header;
> > =20
> >  int __init llc_sysctl_init(void)
> >  {
> > +	struct ctl_table empty[1] =3D {};
> >  	llc2_timeout_header =3D register_net_sysctl(&init_net, "net/llc/llc2/=
timeout", llc2_timeout_table);
> > -	llc_station_header =3D register_net_sysctl(&init_net, "net/llc/statio=
n", llc_station_table);
> > +	llc_station_header =3D register_net_sysctl_sz(&init_net, "net/llc/sta=
tion", empty, 0);
>=20
> Do we really need this ... ??
That is a good question, but its something that needs to be address
outside this patchset. I'm just keeping current behaviour.

>=20
>=20
> > =20
> >  	if (!llc2_timeout_header || !llc_station_header) {
> >  		llc_sysctl_exit();
> > diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
> > index 6dab883a08dd..e163fac55ffa 100644
> > --- a/net/mpls/af_mpls.c
> > +++ b/net/mpls/af_mpls.c
> > @@ -1393,7 +1393,6 @@ static const struct ctl_table mpls_dev_table[] =
=3D {
> >  		.proc_handler	=3D mpls_conf_proc,
> >  		.data		=3D MPLS_PERDEV_SYSCTL_OFFSET(input_enabled),
> >  	},
> > -	{ }
> >  };
> > =20
> >  static int mpls_dev_sysctl_register(struct net_device *dev,
> > @@ -2689,7 +2688,7 @@ static int mpls_net_init(struct net *net)
> >  	/* Table data contains only offsets relative to the base of
> >  	 * the mdev at this point, so make them absolute.
> >  	 */
> > -	for (i =3D 0; i < ARRAY_SIZE(mpls_table) - 1; i++)
> > +	for (i =3D 0; i < ARRAY_SIZE(mpls_table); i++)
>=20
>                         ^^^
>=20
>=20
> >  		table[i].data =3D (char *)net + (uintptr_t)table[i].data;
> > =20
> >  	net->mpls.ctl =3D register_net_sysctl_sz(net, "net/mpls", table,
> > diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
> > index 13fe0748dde8..8bf7c26a0878 100644
> > --- a/net/mptcp/ctrl.c
> > +++ b/net/mptcp/ctrl.c
=2E..
> > =20
> >  	net->xfrm.sysctl_hdr =3D register_net_sysctl_sz(net, "net/core", tabl=
e,
> >  						      table_size);
> >=20
> > --=20
> > 2.43.0
> B

Thx for the review

Best
--=20

Joel Granados

--ebocjdihejx32jpt
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQGzBAABCgAdFiEErkcJVyXmMSXOyyeQupfNUreWQU8FAmYeSNMACgkQupfNUreW
QU8rEQv+MUi7fm5oXY0H9hoW3rH7wTOOXoaWiXD+2m4X9BCFtR+IO7Hbi4TCr+vb
jnK7Yvsbcj8krLpOAr3GlcUtn7MvnzUBu4gR2vQ1PWNrHdZfVF2h+qb6R4lBazhK
Ri7yXSJrCB2IdfryrmFqvFCwBQjVvZv5B8p8vK2BtIRqSN6N2acqMmp5UbfE6HWj
pvFakjOfrMPY6XUabRE3+P3xMHOqlBEtgugMwlLQ++0MG+6URTFeXYt7Av0iTwas
xkK/6Kx7G5p+4qTYrfm48BP51HyIFhzbE5Tapp7ytO3xHvXyRBm04Nw6Gj2Ta7W0
7HlDyKSiZNwGVf2wElQfifKlgJPZUmSdz4x87++8QSPdv8omF8qiR0PSWPSSIjhh
NxKUMRp3TnxCHEu3opx9NODX2JWD5dUoAJ7iCLctAYu7QHk1Oj5uoT8CCmzLwcSJ
IcrwHRFVUjfe5mgmC2MnVB/uvqWPstV1ntAQY6mxXIx3L/QxRC3SlKgEZS4X7jS5
u+CGwYd+
=O6yr
-----END PGP SIGNATURE-----

--ebocjdihejx32jpt--