| Message ID | 20240924050937.697118-1-senozhatsky@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCHv2] mm: do not export const kfree and kstrdup variants | expand |
On (24/09/24 14:08), Sergey Senozhatsky wrote: > Both kfree_const() and kstrdup_const() use __start_rodata > and __end_rodata, which do not work for modules. This is > especially important for kfree_const(). Stop exporting > these functions, as they cannot be used in the modules. Just for linux-mm visibility, consider this patch RFC, more discussions on this at [1] and [2] [1] https://lore.kernel.org/linux-kernel/20240924054151.GL38742@google.com [2] https://lore.kernel.org/linux-kernel/20240924054951.GM38742@google.com
On Tue, Sep 24, 2024 at 02:08:37PM +0900, Sergey Senozhatsky wrote: > Both kfree_const() and kstrdup_const() use __start_rodata > and __end_rodata, which do not work for modules. This is > especially important for kfree_const(). Stop exporting > these functions, as they cannot be used in the modules. Well, they do work when called from modules, they just don't work on constant data that is in modules. There's also plenty of existing callers in modules. So just unexporting them is going to break. The API is kinda horrible, but an implementation to check for constants in modules would also be quite horrible. So I don't have a good answer here, but simply unexporting them is not going to cut it.
On (24/09/23 23:43), Christoph Hellwig wrote: > On Tue, Sep 24, 2024 at 02:08:37PM +0900, Sergey Senozhatsky wrote: > > Both kfree_const() and kstrdup_const() use __start_rodata > > and __end_rodata, which do not work for modules. This is > > especially important for kfree_const(). Stop exporting > > these functions, as they cannot be used in the modules. > > Well, they do work when called from modules, they just don't work > on constant data that is in modules. There's also plenty of > existing callers in modules. > > So just unexporting them is going to break. The API is kinda > horrible, but an implementation to check for constants in modules > would also be quite horrible. So I don't have a good answer here, > but simply unexporting them is not going to cut it. Totally agree with all the points, I haven't looked at how popular that API was before sending out the patch. Is there some sort of "built time const" but for strings that we, perhaps, can add to kfree_const() (and make kfree_const() always inline)? So that we can turn this str = "boom"; ... kfree_const(str); into a safe scenario for modules.
On Tue, Sep 24, 2024 at 03:56:53PM +0900, Sergey Senozhatsky wrote: > Totally agree with all the points, I haven't looked at how > popular that API was before sending out the patch. Is there > some sort of "built time const" but for strings that we, perhaps, > can add to kfree_const() (and make kfree_const() always inline)? > So that we can turn this > > str = "boom"; > ... > kfree_const(str); > > into a safe scenario for modules. Not sure, but even then the API would be horrible as it still would not work for constants in other modules than the one calling it.
Hi Sergey, kernel test robot noticed the following build errors: [auto build test ERROR on akpm-mm/mm-everything] url: https://github.com/intel-lab-lkp/linux/commits/Sergey-Senozhatsky/mm-do-not-export-const-kfree-and-kstrdup-variants/20240924-131016 base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything patch link: https://lore.kernel.org/r/20240924050937.697118-1-senozhatsky%40chromium.org patch subject: [PATCHv2] mm: do not export const kfree and kstrdup variants config: loongarch-randconfig-r061-20240924 (https://download.01.org/0day-ci/archive/20240925/202409250157.TXezi0Gp-lkp@intel.com/config) compiler: loongarch64-linux-gcc (GCC) 14.1.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240925/202409250157.TXezi0Gp-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/202409250157.TXezi0Gp-lkp@intel.com/ All errors (new ones prefixed by >>, old ones prefixed by <<): WARNING: modpost: missing MODULE_DESCRIPTION() in lib/zlib_inflate/zlib_inflate.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/devfreq/governor_powersave.o >> ERROR: modpost: "kstrdup_const" [drivers/dax/kmem.ko] undefined! >> ERROR: modpost: "kfree_const" [drivers/dax/kmem.ko] undefined! >> ERROR: modpost: "kfree_const" [drivers/input/touchscreen/chipone_icn8505.ko] undefined! >> ERROR: modpost: "kstrdup_const" [drivers/power/sequencing/pwrseq-core.ko] undefined! >> ERROR: modpost: "kfree_const" [drivers/power/sequencing/pwrseq-core.ko] undefined! >> ERROR: modpost: "kfree_const" [drivers/memstick/core/memstick.ko] undefined! >> ERROR: modpost: "kfree_const" [net/bluetooth/bluetooth.ko] undefined! >> ERROR: modpost: "kstrdup_const" [net/nfc/hci/hci.ko] undefined! >> ERROR: modpost: "kfree_const" [net/nfc/hci/hci.ko] undefined!
Le 24/09/2024 à 09:02, Christoph Hellwig a écrit : > On Tue, Sep 24, 2024 at 03:56:53PM +0900, Sergey Senozhatsky wrote: >> Totally agree with all the points, I haven't looked at how >> popular that API was before sending out the patch. Is there >> some sort of "built time const" but for strings that we, perhaps, >> can add to kfree_const() (and make kfree_const() always inline)? >> So that we can turn this >> >> str = "boom"; >> ... >> kfree_const(str); >> >> into a safe scenario for modules. > > Not sure, but even then the API would be horrible as it still would > not work for constants in other modules than the one calling it. > > So, the best is to audit, at least code that can be built as a module for str = "boom" pattern, and fix relevant places? Or, considering that these these _const() versions are only there to save a few bytes of memory, wonder if it really worth it? In other word: 1) apply something like the patch below 2) remove the API At a minimum, I think that devm_kstrdup_const() could be removed. CJ diff --git a/mm/util.c b/mm/util.c index 4f1275023eb7..fd5e98fb6362 100644 --- a/mm/util.c +++ b/mm/util.c @@ -39,8 +39,7 @@ */ void kfree_const(const void *x) { - if (!is_kernel_rodata((unsigned long)x)) - kfree(x); + kfree(x); } EXPORT_SYMBOL(kfree_const); @@ -81,9 +80,6 @@ EXPORT_SYMBOL(kstrdup); */ const char *kstrdup_const(const char *s, gfp_t gfp) { - if (is_kernel_rodata((unsigned long)s)) - return s; - return kstrdup(s, gfp); } EXPORT_SYMBOL(kstrdup_const);
diff --git a/mm/util.c b/mm/util.c index 4f1275023eb7..24dadbd5727a 100644 --- a/mm/util.c +++ b/mm/util.c @@ -42,7 +42,6 @@ void kfree_const(const void *x) if (!is_kernel_rodata((unsigned long)x)) kfree(x); } -EXPORT_SYMBOL(kfree_const); /** * kstrdup - allocate space for and copy an existing string @@ -86,7 +85,6 @@ const char *kstrdup_const(const char *s, gfp_t gfp) return kstrdup(s, gfp); } -EXPORT_SYMBOL(kstrdup_const); /** * kstrndup - allocate space for and copy an existing string
Both kfree_const() and kstrdup_const() use __start_rodata and __end_rodata, which do not work for modules. This is especially important for kfree_const(). Stop exporting these functions, as they cannot be used in the modules. Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> --- mm/util.c | 2 -- 1 file changed, 2 deletions(-)