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(-)

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
-- 
2.46.0.792.g87dc391469-goog

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!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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.

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);

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