| Message ID | 20240328-jag-sysctl_remset_net-v2-4-52c9fad9a1af@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,1/4] networking: Remove the now superfluous sentinel elements from ctl_table array | expand |
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
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
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
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
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!
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
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 --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);