diff mbox series

[v2,4/4] ax.25: Remove the now superfluous sentinel elements from ctl_table array

Message ID 20240328-jag-sysctl_remset_net-v2-4-52c9fad9a1af@samsung.com (mailing list archive)
State Superseded
Headers show
Series [v2,1/4] networking: Remove the now superfluous sentinel elements from ctl_table array | expand

Commit Message

Joel Granados via B4 Relay March 28, 2024, 3:40 p.m. UTC
From: Joel Granados <j.granados@samsung.com>

This commit comes at the tail end of a greater effort to remove the
empty elements at the end of the ctl_table arrays (sentinels) which will
reduce the overall build time size of the kernel and run time memory
bloat by ~64 bytes per sentinel (further information Link :
https://lore.kernel.org/all/ZO5Yx5JFogGi%2FcBo@bombadil.infradead.org/)

When we remove the sentinel from ax25_param_table a buffer overflow
shows its ugly head. The sentinel's data element used to be changed when
CONFIG_AX25_DAMA_SLAVE was not defined. This did not have any adverse
effects as we still stopped on the sentinel because of its null
procname. But now that we do not have the sentinel element, we are
careful to check ax25_param_table's size.

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 net/ax25/sysctl_net_ax25.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Kuniyuki Iwashima March 28, 2024, 7:49 p.m. UTC | #1
From: Joel Granados via B4 Relay <devnull+j.granados.samsung.com@kernel.org>
Date: Thu, 28 Mar 2024 16:40:05 +0100
> This commit comes at the tail end of a greater effort to remove the
> empty elements at the end of the ctl_table arrays (sentinels) which will
> reduce the overall build time size of the kernel and run time memory
> bloat by ~64 bytes per sentinel (further information Link :
> https://lore.kernel.org/all/ZO5Yx5JFogGi%2FcBo@bombadil.infradead.org/)
> 
> When we remove the sentinel from ax25_param_table a buffer overflow
> shows its ugly head. The sentinel's data element used to be changed when
> CONFIG_AX25_DAMA_SLAVE was not defined.

I think it's better to define the relation explicitly between the
enum and sysctl table by BUILD_BUG_ON() in ax25_register_dev_sysctl()

  BUILD_BUG_ON(AX25_MAX_VALUES != ARRAY_SIZE(ax25_param_table));

and guard AX25_VALUES_DS_TIMEOUT with #ifdef CONFIG_AX25_DAMA_SLAVE
as done for other enum.


> This did not have any adverse
> effects as we still stopped on the sentinel because of its null
> procname. But now that we do not have the sentinel element, we are
> careful to check ax25_param_table's size.
> 
> Signed-off-by: Joel Granados <j.granados@samsung.com>
> ---
>  net/ax25/sysctl_net_ax25.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/net/ax25/sysctl_net_ax25.c b/net/ax25/sysctl_net_ax25.c
> index db66e11e7fe8..e55be8817a1e 100644
> --- a/net/ax25/sysctl_net_ax25.c
> +++ b/net/ax25/sysctl_net_ax25.c
> @@ -141,8 +141,6 @@ static const struct ctl_table ax25_param_table[] = {
>  		.extra2		= &max_ds_timeout
>  	},
>  #endif
> -
> -	{ }	/* that's all, folks! */
>  };
>  
>  int ax25_register_dev_sysctl(ax25_dev *ax25_dev)
> @@ -155,7 +153,7 @@ int ax25_register_dev_sysctl(ax25_dev *ax25_dev)
>  	if (!table)
>  		return -ENOMEM;
>  
> -	for (k = 0; k < AX25_MAX_VALUES; k++)
> +	for (k = 0; k < AX25_MAX_VALUES && k < ARRAY_SIZE(ax25_param_table); k++)
>  		table[k].data = &ax25_dev->values[k];
>  
>  	snprintf(path, sizeof(path), "net/ax25/%s", ax25_dev->dev->name);
> 
> -- 
> 2.43.0
Joel Granados April 3, 2024, 9:16 a.m. UTC | #2
On Thu, Mar 28, 2024 at 12:49:34PM -0700, Kuniyuki Iwashima wrote:
> From: Joel Granados via B4 Relay <devnull+j.granados.samsung.com@kernel.org>
> Date: Thu, 28 Mar 2024 16:40:05 +0100
> > This commit comes at the tail end of a greater effort to remove the
> > empty elements at the end of the ctl_table arrays (sentinels) which will
> > reduce the overall build time size of the kernel and run time memory
> > bloat by ~64 bytes per sentinel (further information Link :
> > https://lore.kernel.org/all/ZO5Yx5JFogGi%2FcBo@bombadil.infradead.org/)
> > 
> > When we remove the sentinel from ax25_param_table a buffer overflow
> > shows its ugly head. The sentinel's data element used to be changed when
> > CONFIG_AX25_DAMA_SLAVE was not defined.
> 
> I think it's better to define the relation explicitly between the
> enum and sysctl table by BUILD_BUG_ON() in ax25_register_dev_sysctl()
> 
>   BUILD_BUG_ON(AX25_MAX_VALUES != ARRAY_SIZE(ax25_param_table));
> 
> and guard AX25_VALUES_DS_TIMEOUT with #ifdef CONFIG_AX25_DAMA_SLAVE
> as done for other enum.
This is a great idea. I'll also try to look and see where else I can add
the explicit relation check.

Thx for the review

> 
> 
> > This did not have any adverse
> > effects as we still stopped on the sentinel because of its null
> > procname. But now that we do not have the sentinel element, we are
> > careful to check ax25_param_table's size.
> > 
> > Signed-off-by: Joel Granados <j.granados@samsung.com>
> > ---
> >  net/ax25/sysctl_net_ax25.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/net/ax25/sysctl_net_ax25.c b/net/ax25/sysctl_net_ax25.c
> > index db66e11e7fe8..e55be8817a1e 100644
> > --- a/net/ax25/sysctl_net_ax25.c
> > +++ b/net/ax25/sysctl_net_ax25.c
> > @@ -141,8 +141,6 @@ static const struct ctl_table ax25_param_table[] = {
> >  		.extra2		= &max_ds_timeout
> >  	},
> >  #endif
> > -
> > -	{ }	/* that's all, folks! */
> >  };
> >  
> >  int ax25_register_dev_sysctl(ax25_dev *ax25_dev)
> > @@ -155,7 +153,7 @@ int ax25_register_dev_sysctl(ax25_dev *ax25_dev)
> >  	if (!table)
> >  		return -ENOMEM;
> >  
> > -	for (k = 0; k < AX25_MAX_VALUES; k++)
> > +	for (k = 0; k < AX25_MAX_VALUES && k < ARRAY_SIZE(ax25_param_table); k++)
> >  		table[k].data = &ax25_dev->values[k];
> >  
> >  	snprintf(path, sizeof(path), "net/ax25/%s", ax25_dev->dev->name);
> > 
> > -- 
> > 2.43.0
Joel Granados April 3, 2024, 10:59 a.m. UTC | #3
On Wed, Apr 03, 2024 at 11:16:25AM +0200, Joel Granados wrote:
> On Thu, Mar 28, 2024 at 12:49:34PM -0700, Kuniyuki Iwashima wrote:
> > From: Joel Granados via B4 Relay <devnull+j.granados.samsung.com@kernel.org>
> > Date: Thu, 28 Mar 2024 16:40:05 +0100
> > > This commit comes at the tail end of a greater effort to remove the
> > > empty elements at the end of the ctl_table arrays (sentinels) which will
> > > reduce the overall build time size of the kernel and run time memory
> > > bloat by ~64 bytes per sentinel (further information Link :
> > > https://lore.kernel.org/all/ZO5Yx5JFogGi%2FcBo@bombadil.infradead.org/)
> > > 
> > > When we remove the sentinel from ax25_param_table a buffer overflow
> > > shows its ugly head. The sentinel's data element used to be changed when
> > > CONFIG_AX25_DAMA_SLAVE was not defined.
> > 
> > I think it's better to define the relation explicitly between the
> > enum and sysctl table by BUILD_BUG_ON() in ax25_register_dev_sysctl()
> > 
> >   BUILD_BUG_ON(AX25_MAX_VALUES != ARRAY_SIZE(ax25_param_table));
> > 
> > and guard AX25_VALUES_DS_TIMEOUT with #ifdef CONFIG_AX25_DAMA_SLAVE
> > as done for other enum.
> This is a great idea. I'll also try to look and see where else I can add
> the explicit relation check.
> 
> Thx for the review
> 
> > 
> > 
> > > This did not have any adverse
> > > effects as we still stopped on the sentinel because of its null
> > > procname. But now that we do not have the sentinel element, we are
> > > careful to check ax25_param_table's size.
> > > 
> > > Signed-off-by: Joel Granados <j.granados@samsung.com>
> > > ---
> > >  net/ax25/sysctl_net_ax25.c | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/net/ax25/sysctl_net_ax25.c b/net/ax25/sysctl_net_ax25.c
> > > index db66e11e7fe8..e55be8817a1e 100644
> > > --- a/net/ax25/sysctl_net_ax25.c
> > > +++ b/net/ax25/sysctl_net_ax25.c
> > > @@ -141,8 +141,6 @@ static const struct ctl_table ax25_param_table[] = {
> > >  		.extra2		= &max_ds_timeout
> > >  	},
> > >  #endif
> > > -
> > > -	{ }	/* that's all, folks! */
> > >  };
> > >  
> > >  int ax25_register_dev_sysctl(ax25_dev *ax25_dev)
> > > @@ -155,7 +153,7 @@ int ax25_register_dev_sysctl(ax25_dev *ax25_dev)
> > >  	if (!table)
> > >  		return -ENOMEM;
> > >  
> > > -	for (k = 0; k < AX25_MAX_VALUES; k++)
> > > +	for (k = 0; k < AX25_MAX_VALUES && k < ARRAY_SIZE(ax25_param_table); k++)
And with the BUILD_BUG_ON we don't need to do the `k <
ARRAY_SIZE(ax25_param_table)` any longer. Win/win :)

> > >  		table[k].data = &ax25_dev->values[k];
> > >  
> > >  	snprintf(path, sizeof(path), "net/ax25/%s", ax25_dev->dev->name);
> > > 
> > > -- 
> > > 2.43.0
> 
> -- 
> 
> Joel Granados
Joel Granados April 5, 2024, 7:15 a.m. UTC | #4
On Thu, Mar 28, 2024 at 12:49:34PM -0700, Kuniyuki Iwashima wrote:
> From: Joel Granados via B4 Relay <devnull+j.granados.samsung.com@kernel.org>
> Date: Thu, 28 Mar 2024 16:40:05 +0100
> > This commit comes at the tail end of a greater effort to remove the
> > empty elements at the end of the ctl_table arrays (sentinels) which will
> > reduce the overall build time size of the kernel and run time memory
> > bloat by ~64 bytes per sentinel (further information Link :
> > https://lore.kernel.org/all/ZO5Yx5JFogGi%2FcBo@bombadil.infradead.org/)
> > 
> > When we remove the sentinel from ax25_param_table a buffer overflow
> > shows its ugly head. The sentinel's data element used to be changed when
> > CONFIG_AX25_DAMA_SLAVE was not defined.
> 
> I think it's better to define the relation explicitly between the
> enum and sysctl table by BUILD_BUG_ON() in ax25_register_dev_sysctl()
> 
>   BUILD_BUG_ON(AX25_MAX_VALUES != ARRAY_SIZE(ax25_param_table));
> 
> and guard AX25_VALUES_DS_TIMEOUT with #ifdef CONFIG_AX25_DAMA_SLAVE
> as done for other enum.

When I remove AX25_VALUES_DS_TIMEOUT from the un-guarded build it
complains in net/ax25/ax25_ds_timer.c (ax25_ds_set_timer). Here is the
report https://lore.kernel.org/oe-kbuild-all/202404040301.qzKmVQGB-lkp@intel.com/.

How best to address this? Should we just guard the whole function and do
nothing when not set? like this:

```
void ax25_ds_set_timer(ax25_dev *ax25_dev)
{
#ifdef COFNIG_AX25_DAMA_SLAVE
        if (ax25_dev == NULL)        ···/* paranoia */
                return;

        ax25_dev->dama.slave_timeout =
                msecs_to_jiffies(ax25_dev->values[AX25_VALUES_DS_TIMEOUT]) / 10;
        mod_timer(&ax25_dev->dama.slave_timer, jiffies + HZ);
#else
        return;
#endif
}

```

I'm not too familiar with this, so pointing me to the "correct" way to
handle this would be helpfull.

Thx in advance.

Best
Kuniyuki Iwashima April 5, 2024, 10:26 p.m. UTC | #5
From: Joel Granados <j.granados@samsung.com>
Date: Fri, 5 Apr 2024 09:15:31 +0200
> On Thu, Mar 28, 2024 at 12:49:34PM -0700, Kuniyuki Iwashima wrote:
> > From: Joel Granados via B4 Relay <devnull+j.granados.samsung.com@kernel.org>
> > Date: Thu, 28 Mar 2024 16:40:05 +0100
> > > This commit comes at the tail end of a greater effort to remove the
> > > empty elements at the end of the ctl_table arrays (sentinels) which will
> > > reduce the overall build time size of the kernel and run time memory
> > > bloat by ~64 bytes per sentinel (further information Link :
> > > https://lore.kernel.org/all/ZO5Yx5JFogGi%2FcBo@bombadil.infradead.org/)
> > > 
> > > When we remove the sentinel from ax25_param_table a buffer overflow
> > > shows its ugly head. The sentinel's data element used to be changed when
> > > CONFIG_AX25_DAMA_SLAVE was not defined.
> > 
> > I think it's better to define the relation explicitly between the
> > enum and sysctl table by BUILD_BUG_ON() in ax25_register_dev_sysctl()
> > 
> >   BUILD_BUG_ON(AX25_MAX_VALUES != ARRAY_SIZE(ax25_param_table));
> > 
> > and guard AX25_VALUES_DS_TIMEOUT with #ifdef CONFIG_AX25_DAMA_SLAVE
> > as done for other enum.
> 
> When I remove AX25_VALUES_DS_TIMEOUT from the un-guarded build it
> complains in net/ax25/ax25_ds_timer.c (ax25_ds_set_timer). Here is the
> report https://lore.kernel.org/oe-kbuild-all/202404040301.qzKmVQGB-lkp@intel.com/.
> 
> How best to address this? Should we just guard the whole function and do
> nothing when not set? like this:

It seems fine to me.

ax25_ds_timeout() checks !ax25_dev->dama.slave_timeout, but it's
initialised by kzalloc() during dev setup, so it will be a noop.


> 
> ```
> void ax25_ds_set_timer(ax25_dev *ax25_dev)
> {
> #ifdef COFNIG_AX25_DAMA_SLAVE
>         if (ax25_dev == NULL)        ···/* paranoia */
>                 return;
> 
>         ax25_dev->dama.slave_timeout =
>                 msecs_to_jiffies(ax25_dev->values[AX25_VALUES_DS_TIMEOUT]) / 10;
>         mod_timer(&ax25_dev->dama.slave_timer, jiffies + HZ);
> #else
>         return;
> #endif
> }
> 
> ```
> 
> I'm not too familiar with this, so pointing me to the "correct" way to
> handle this would be helpfull.

Also, you will need to guard another use of AX25_VALUES_DS_TIMEOUT in
ax25_dev_device_up().

Thanks!
Joel Granados April 8, 2024, 7:20 a.m. UTC | #6
On Fri, Apr 05, 2024 at 03:26:58PM -0700, Kuniyuki Iwashima wrote:
> From: Joel Granados <j.granados@samsung.com>
> Date: Fri, 5 Apr 2024 09:15:31 +0200
> > On Thu, Mar 28, 2024 at 12:49:34PM -0700, Kuniyuki Iwashima wrote:
> > > From: Joel Granados via B4 Relay <devnull+j.granados.samsung.com@kernel.org>
> > > Date: Thu, 28 Mar 2024 16:40:05 +0100
> > > > This commit comes at the tail end of a greater effort to remove the
> > > > empty elements at the end of the ctl_table arrays (sentinels) which will
> > > > reduce the overall build time size of the kernel and run time memory
> > > > bloat by ~64 bytes per sentinel (further information Link :
> > > > https://lore.kernel.org/all/ZO5Yx5JFogGi%2FcBo@bombadil.infradead.org/)
> > > > 
> > > > When we remove the sentinel from ax25_param_table a buffer overflow
> > > > shows its ugly head. The sentinel's data element used to be changed when
> > > > CONFIG_AX25_DAMA_SLAVE was not defined.
> > > 
> > > I think it's better to define the relation explicitly between the
> > > enum and sysctl table by BUILD_BUG_ON() in ax25_register_dev_sysctl()
> > > 
> > >   BUILD_BUG_ON(AX25_MAX_VALUES != ARRAY_SIZE(ax25_param_table));
> > > 
> > > and guard AX25_VALUES_DS_TIMEOUT with #ifdef CONFIG_AX25_DAMA_SLAVE
> > > as done for other enum.
> > 
> > When I remove AX25_VALUES_DS_TIMEOUT from the un-guarded build it
> > complains in net/ax25/ax25_ds_timer.c (ax25_ds_set_timer). Here is the
> > report https://lore.kernel.org/oe-kbuild-all/202404040301.qzKmVQGB-lkp@intel.com/.
> > 
> > How best to address this? Should we just guard the whole function and do
> > nothing when not set? like this:
> 
> It seems fine to me.
> 
> ax25_ds_timeout() checks !ax25_dev->dama.slave_timeout, but it's
> initialised by kzalloc() during dev setup, so it will be a noop.
thx. I'll solve it like this then

> 
> 
> > 
> > ```
> > void ax25_ds_set_timer(ax25_dev *ax25_dev)
> > {
> > #ifdef COFNIG_AX25_DAMA_SLAVE
> >         if (ax25_dev == NULL)        ···/* paranoia */
> >                 return;
> > 
> >         ax25_dev->dama.slave_timeout =
> >                 msecs_to_jiffies(ax25_dev->values[AX25_VALUES_DS_TIMEOUT]) / 10;
> >         mod_timer(&ax25_dev->dama.slave_timer, jiffies + HZ);
> > #else
> >         return;
> > #endif
> > }
> > 
> > ```
> > 
> > I'm not too familiar with this, so pointing me to the "correct" way to
> > handle this would be helpfull.
> 
> Also, you will need to guard another use of AX25_VALUES_DS_TIMEOUT in
> ax25_dev_device_up().
Yes. I had noticed this already. This was a trivial one though, so I did
not ask about it.

Thx.

Best
Joel Granados April 12, 2024, 2:50 p.m. UTC | #7
On Fri, Apr 05, 2024 at 03:26:58PM -0700, Kuniyuki Iwashima wrote:
> From: Joel Granados <j.granados@samsung.com>
> Date: Fri, 5 Apr 2024 09:15:31 +0200
> > On Thu, Mar 28, 2024 at 12:49:34PM -0700, Kuniyuki Iwashima wrote:
> > > From: Joel Granados via B4 Relay <devnull+j.granados.samsung.com@kernel.org>
> > > Date: Thu, 28 Mar 2024 16:40:05 +0100
> > > > This commit comes at the tail end of a greater effort to remove the
> > > > empty elements at the end of the ctl_table arrays (sentinels) which will
> > > > reduce the overall build time size of the kernel and run time memory
> > > > bloat by ~64 bytes per sentinel (further information Link :
> > > > https://lore.kernel.org/all/ZO5Yx5JFogGi%2FcBo@bombadil.infradead.org/)
> > > > 
> > > > When we remove the sentinel from ax25_param_table a buffer overflow
> > > > shows its ugly head. The sentinel's data element used to be changed when
> > > > CONFIG_AX25_DAMA_SLAVE was not defined.
> > > 
> > > I think it's better to define the relation explicitly between the
> > > enum and sysctl table by BUILD_BUG_ON() in ax25_register_dev_sysctl()
> > > 
> > >   BUILD_BUG_ON(AX25_MAX_VALUES != ARRAY_SIZE(ax25_param_table));
> > > 
> > > and guard AX25_VALUES_DS_TIMEOUT with #ifdef CONFIG_AX25_DAMA_SLAVE
> > > as done for other enum.
> > 
> > When I remove AX25_VALUES_DS_TIMEOUT from the un-guarded build it
> > complains in net/ax25/ax25_ds_timer.c (ax25_ds_set_timer). Here is the
> > report https://lore.kernel.org/oe-kbuild-all/202404040301.qzKmVQGB-lkp@intel.com/.
> > 
> > How best to address this? Should we just guard the whole function and do
> > nothing when not set? like this:
> 
> It seems fine to me.
> 
> ax25_ds_timeout() checks !ax25_dev->dama.slave_timeout, but it's
> initialised by kzalloc() during dev setup, so it will be a noop.

Just sent v3 with this change.
diff mbox series

Patch

diff --git a/net/ax25/sysctl_net_ax25.c b/net/ax25/sysctl_net_ax25.c
index db66e11e7fe8..e55be8817a1e 100644
--- a/net/ax25/sysctl_net_ax25.c
+++ b/net/ax25/sysctl_net_ax25.c
@@ -141,8 +141,6 @@  static const struct ctl_table ax25_param_table[] = {
 		.extra2		= &max_ds_timeout
 	},
 #endif
-
-	{ }	/* that's all, folks! */
 };
 
 int ax25_register_dev_sysctl(ax25_dev *ax25_dev)
@@ -155,7 +153,7 @@  int ax25_register_dev_sysctl(ax25_dev *ax25_dev)
 	if (!table)
 		return -ENOMEM;
 
-	for (k = 0; k < AX25_MAX_VALUES; k++)
+	for (k = 0; k < AX25_MAX_VALUES && k < ARRAY_SIZE(ax25_param_table); k++)
 		table[k].data = &ax25_dev->values[k];
 
 	snprintf(path, sizeof(path), "net/ax25/%s", ax25_dev->dev->name);