| Message ID | 20191004095748.223119-1-maennich@google.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | depmod: create and use System.map.no_namespaces | expand |
On Fri, Oct 4, 2019 at 2:57 AM Matthias Maennich <maennich@google.com> wrote: > > depmod in its current version is not aware of symbol namespace in > ksymtab entries introduced with 8651ec01daed ("module: add support for > symbol namespaces."). They have the form > > __ksymtab_NAMESPACE.symbol_name > > A fix for kmod's depmod has been proposed [1]. In order to support older > versions of depmod as well, create a System.map.no_namespaces during > scripts/depmod.sh that has the pre-namespaces format. That way users do > not immediately upgrade the userspace tool. > > [1] https://lore.kernel.org/linux-modules/20191004094136.166621-1-maennich@google.com/ > > Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > Fixes: 8651ec01daed ("module: add support for symbol namespaces.") > Cc: Masahiro Yamada <yamada.masahiro@socionext.com> > Cc: Lucas De Marchi <lucas.de.marchi@gmail.com> > Cc: Jessica Yu <jeyu@kernel.org> > Cc: Martijn Coenen <maco@android.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: linux-modules@vger.kernel.org > Signed-off-by: Matthias Maennich <maennich@google.com> > --- > > Please note this depends on the new ksymtab entry format proposed in > https://lore.kernel.org/lkml/20191003075826.7478-2-yamada.masahiro@socionext.com/ I don't really agree with that thought, more below. > > That is likely to be merged soon as well as it fixes problems in 5.4-rc*, hence > this patch depends on it. > > Cheers, > Matthias > > .gitignore | 1 + > scripts/depmod.sh | 8 +++++++- > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/.gitignore b/.gitignore > index 70580bdd352c..5ed58a7cb433 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -59,6 +59,7 @@ modules.order > /vmlinux-gdb.py > /vmlinuz > /System.map > +/System.map.no_namespaces > /Module.markers > /modules.builtin.modinfo > > diff --git a/scripts/depmod.sh b/scripts/depmod.sh > index e083bcae343f..602e1af072c7 100755 > --- a/scripts/depmod.sh > +++ b/scripts/depmod.sh > @@ -39,7 +39,13 @@ if $depmod_hack_needed; then > KERNELRELEASE=99.98.$KERNELRELEASE > fi > > -set -- -ae -F System.map > +# Older versions of depmod do not support symbol namespaces in ksymtab entries, > +# hence create an alternative System.map with namespace patched out to use for > +# depmod. I.e. transform entries as follows: > +# __ksymtab_NAMESPACE.symbol_name -> __ksymtab_symbol_name > +sed 's/__ksymtab_.*\./__ksymtab_/' System.map > System.map.no_namespaces So people with old kmod will have to know they need to pass System.map.no_namespaces rather than the usual System.map. Also, distros will need to be update to also copy the new file to the kernel package (or upgrade/patch kmod). I'd rather maintain the current format and fix the bug that patch is fixing. The namespace in the end IMO is just a small annoyance with a reason to exist. Lucas De Marchi > + > +set -- -ae -F System.map.no_namespaces > if test -n "$INSTALL_MOD_PATH"; then > set -- "$@" -b "$INSTALL_MOD_PATH" > fi > -- > 2.23.0.581.g78d2f28ef7-goog >
On Sat, Oct 5, 2019 at 3:25 AM Lucas De Marchi <lucas.de.marchi@gmail.com> wrote: > > On Fri, Oct 4, 2019 at 2:57 AM Matthias Maennich <maennich@google.com> wrote: > > > > depmod in its current version is not aware of symbol namespace in > > ksymtab entries introduced with 8651ec01daed ("module: add support for > > symbol namespaces."). They have the form > > > > __ksymtab_NAMESPACE.symbol_name > > > > A fix for kmod's depmod has been proposed [1]. In order to support older > > versions of depmod as well, create a System.map.no_namespaces during > > scripts/depmod.sh that has the pre-namespaces format. That way users do > > not immediately upgrade the userspace tool. > > > > [1] https://lore.kernel.org/linux-modules/20191004094136.166621-1-maennich@google.com/ > > > > Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > > Fixes: 8651ec01daed ("module: add support for symbol namespaces.") > > Cc: Masahiro Yamada <yamada.masahiro@socionext.com> > > Cc: Lucas De Marchi <lucas.de.marchi@gmail.com> > > Cc: Jessica Yu <jeyu@kernel.org> > > Cc: Martijn Coenen <maco@android.com> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: linux-modules@vger.kernel.org > > Signed-off-by: Matthias Maennich <maennich@google.com> > > --- > > > > Please note this depends on the new ksymtab entry format proposed in > > https://lore.kernel.org/lkml/20191003075826.7478-2-yamada.masahiro@socionext.com/ > > I don't really agree with that thought, more below. > > > > > That is likely to be merged soon as well as it fixes problems in 5.4-rc*, hence > > this patch depends on it. > > > > Cheers, > > Matthias > > > > .gitignore | 1 + > > scripts/depmod.sh | 8 +++++++- > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/.gitignore b/.gitignore > > index 70580bdd352c..5ed58a7cb433 100644 > > --- a/.gitignore > > +++ b/.gitignore > > @@ -59,6 +59,7 @@ modules.order > > /vmlinux-gdb.py > > /vmlinuz > > /System.map > > +/System.map.no_namespaces > > /Module.markers > > /modules.builtin.modinfo > > > > diff --git a/scripts/depmod.sh b/scripts/depmod.sh > > index e083bcae343f..602e1af072c7 100755 > > --- a/scripts/depmod.sh > > +++ b/scripts/depmod.sh > > @@ -39,7 +39,13 @@ if $depmod_hack_needed; then > > KERNELRELEASE=99.98.$KERNELRELEASE > > fi > > > > -set -- -ae -F System.map > > +# Older versions of depmod do not support symbol namespaces in ksymtab entries, > > +# hence create an alternative System.map with namespace patched out to use for > > +# depmod. I.e. transform entries as follows: > > +# __ksymtab_NAMESPACE.symbol_name -> __ksymtab_symbol_name > > +sed 's/__ksymtab_.*\./__ksymtab_/' System.map > System.map.no_namespaces > > So people with old kmod will have to know they need to pass > System.map.no_namespaces rather than the usual > System.map. Also, distros will need to be update to also copy the new > file to the kernel package (or upgrade/patch kmod). > > I'd rather maintain the current format and fix the bug that patch is > fixing. The namespace > in the end IMO is just a small annoyance with a reason to exist. I agree, this fix is bad. We should not bother kmod or any tools. And System.map.no_namespaces is a cheesy workaround. BTW, I expressed my negative opinion in the review process for the patch set. I am still not convinced with the namespace feature, but anyway it was merged (with poor review and test). Get back on track, probably the right fix would be to stop using __ksymtab_<namespace>.<symbol>. It is not used for any purposes but passing <namespace> / <symbol> pairs to modpost. For example, __kstrtabns_##sym points to the namespace string, so it would be possible to parse it from modpost? Then, asm("__ksymtab_" #ns NS_SEPARATOR #sym) will go away. Masahiro > Lucas De Marchi > > > + > > +set -- -ae -F System.map.no_namespaces > > if test -n "$INSTALL_MOD_PATH"; then > > set -- "$@" -b "$INSTALL_MOD_PATH" > > fi > > -- > > 2.23.0.581.g78d2f28ef7-goog > > > > > -- > Lucas De Marchi
Hi! On Sat, Oct 05, 2019 at 04:53:56PM +0900, Masahiro Yamada wrote: >On Sat, Oct 5, 2019 at 3:25 AM Lucas De Marchi ><lucas.de.marchi@gmail.com> wrote: >> >> On Fri, Oct 4, 2019 at 2:57 AM Matthias Maennich <maennich@google.com> wrote: >> > >> > depmod in its current version is not aware of symbol namespace in >> > ksymtab entries introduced with 8651ec01daed ("module: add support for >> > symbol namespaces."). They have the form >> > >> > __ksymtab_NAMESPACE.symbol_name >> > >> > A fix for kmod's depmod has been proposed [1]. In order to support older >> > versions of depmod as well, create a System.map.no_namespaces during >> > scripts/depmod.sh that has the pre-namespaces format. That way users do >> > not immediately upgrade the userspace tool. >> > >> > [1] https://lore.kernel.org/linux-modules/20191004094136.166621-1-maennich@google.com/ >> > >> > Reported-by: Stefan Wahren <stefan.wahren@i2se.com> >> > Fixes: 8651ec01daed ("module: add support for symbol namespaces.") >> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com> >> > Cc: Lucas De Marchi <lucas.de.marchi@gmail.com> >> > Cc: Jessica Yu <jeyu@kernel.org> >> > Cc: Martijn Coenen <maco@android.com> >> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> > Cc: linux-modules@vger.kernel.org >> > Signed-off-by: Matthias Maennich <maennich@google.com> >> > --- >> > >> > Please note this depends on the new ksymtab entry format proposed in >> > https://lore.kernel.org/lkml/20191003075826.7478-2-yamada.masahiro@socionext.com/ >> >> I don't really agree with that thought, more below. >> >> > >> > That is likely to be merged soon as well as it fixes problems in 5.4-rc*, hence >> > this patch depends on it. >> > >> > Cheers, >> > Matthias >> > >> > .gitignore | 1 + >> > scripts/depmod.sh | 8 +++++++- >> > 2 files changed, 8 insertions(+), 1 deletion(-) >> > >> > diff --git a/.gitignore b/.gitignore >> > index 70580bdd352c..5ed58a7cb433 100644 >> > --- a/.gitignore >> > +++ b/.gitignore >> > @@ -59,6 +59,7 @@ modules.order >> > /vmlinux-gdb.py >> > /vmlinuz >> > /System.map >> > +/System.map.no_namespaces >> > /Module.markers >> > /modules.builtin.modinfo >> > >> > diff --git a/scripts/depmod.sh b/scripts/depmod.sh >> > index e083bcae343f..602e1af072c7 100755 >> > --- a/scripts/depmod.sh >> > +++ b/scripts/depmod.sh >> > @@ -39,7 +39,13 @@ if $depmod_hack_needed; then >> > KERNELRELEASE=99.98.$KERNELRELEASE >> > fi >> > >> > -set -- -ae -F System.map >> > +# Older versions of depmod do not support symbol namespaces in ksymtab entries, >> > +# hence create an alternative System.map with namespace patched out to use for >> > +# depmod. I.e. transform entries as follows: >> > +# __ksymtab_NAMESPACE.symbol_name -> __ksymtab_symbol_name >> > +sed 's/__ksymtab_.*\./__ksymtab_/' System.map > System.map.no_namespaces >> >> So people with old kmod will have to know they need to pass >> System.map.no_namespaces rather than the usual >> System.map. Also, distros will need to be update to also copy the new >> file to the kernel package (or upgrade/patch kmod). >> >> I'd rather maintain the current format and fix the bug that patch is >> fixing. The namespace >> in the end IMO is just a small annoyance with a reason to exist. > >I agree, this fix is bad. >We should not bother kmod or any tools. >And System.map.no_namespaces is a cheesy workaround. Thanks for the feedback! Based on the discussion from last year about changing kmod/depmod for this change to the ksymtab entries, (https://lore.kernel.org/lkml/CAKi4VA+wvVxPxVSP6ruSR++TYvavKPNAQ5XhREF_9ZxeOKQirg@mail.gmail.com/), I assumed this approach would be acceptable. The workaround (yeah, it is a hack) is a way to mitigate the issue for people that can't update depmod so easily. System.map.no_namespaces was not intended to be part of any distribution, but rather used locally in depmod.sh. I could have made this more clear. > >BTW, I expressed my negative opinion in the review process >for the patch set. I am still not convinced with the >namespace feature, but anyway it was merged >(with poor review and test). > > > >Get back on track, probably the right fix would be to >stop using __ksymtab_<namespace>.<symbol>. > >It is not used for any purposes but passing ><namespace> / <symbol> pairs to modpost. > > >For example, __kstrtabns_##sym points to >the namespace string, so it would be possible >to parse it from modpost? > >Then, asm("__ksymtab_" #ns NS_SEPARATOR #sym) >will go away. I will give this a try. Cheers, Matthias > > >Masahiro > > > > >> Lucas De Marchi >> >> > + >> > +set -- -ae -F System.map.no_namespaces >> > if test -n "$INSTALL_MOD_PATH"; then >> > set -- "$@" -b "$INSTALL_MOD_PATH" >> > fi >> > -- >> > 2.23.0.581.g78d2f28ef7-goog >> > >> >> >> -- >> Lucas De Marchi > > > >-- >Best Regards >Masahiro Yamada
+++ Matthias Maennich [07/10/19 07:46 +0100]: >Hi! > >On Sat, Oct 05, 2019 at 04:53:56PM +0900, Masahiro Yamada wrote: >>On Sat, Oct 5, 2019 at 3:25 AM Lucas De Marchi >><lucas.de.marchi@gmail.com> wrote: >>> >>>On Fri, Oct 4, 2019 at 2:57 AM Matthias Maennich <maennich@google.com> wrote: >>>> >>>> depmod in its current version is not aware of symbol namespace in >>>> ksymtab entries introduced with 8651ec01daed ("module: add support for >>>> symbol namespaces."). They have the form >>>> >>>> __ksymtab_NAMESPACE.symbol_name >>>> >>>> A fix for kmod's depmod has been proposed [1]. In order to support older >>>> versions of depmod as well, create a System.map.no_namespaces during >>>> scripts/depmod.sh that has the pre-namespaces format. That way users do >>>> not immediately upgrade the userspace tool. >>>> >>>> [1] https://lore.kernel.org/linux-modules/20191004094136.166621-1-maennich@google.com/ >>>> >>>> Reported-by: Stefan Wahren <stefan.wahren@i2se.com> >>>> Fixes: 8651ec01daed ("module: add support for symbol namespaces.") >>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com> >>>> Cc: Lucas De Marchi <lucas.de.marchi@gmail.com> >>>> Cc: Jessica Yu <jeyu@kernel.org> >>>> Cc: Martijn Coenen <maco@android.com> >>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>>> Cc: linux-modules@vger.kernel.org >>>> Signed-off-by: Matthias Maennich <maennich@google.com> >>>> --- >>>> >>>> Please note this depends on the new ksymtab entry format proposed in >>>> https://lore.kernel.org/lkml/20191003075826.7478-2-yamada.masahiro@socionext.com/ >>> >>>I don't really agree with that thought, more below. >>> >>>> >>>> That is likely to be merged soon as well as it fixes problems in 5.4-rc*, hence >>>> this patch depends on it. >>>> >>>> Cheers, >>>> Matthias >>>> >>>> .gitignore | 1 + >>>> scripts/depmod.sh | 8 +++++++- >>>> 2 files changed, 8 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/.gitignore b/.gitignore >>>> index 70580bdd352c..5ed58a7cb433 100644 >>>> --- a/.gitignore >>>> +++ b/.gitignore >>>> @@ -59,6 +59,7 @@ modules.order >>>> /vmlinux-gdb.py >>>> /vmlinuz >>>> /System.map >>>> +/System.map.no_namespaces >>>> /Module.markers >>>> /modules.builtin.modinfo >>>> >>>> diff --git a/scripts/depmod.sh b/scripts/depmod.sh >>>> index e083bcae343f..602e1af072c7 100755 >>>> --- a/scripts/depmod.sh >>>> +++ b/scripts/depmod.sh >>>> @@ -39,7 +39,13 @@ if $depmod_hack_needed; then >>>> KERNELRELEASE=99.98.$KERNELRELEASE >>>> fi >>>> >>>> -set -- -ae -F System.map >>>> +# Older versions of depmod do not support symbol namespaces in ksymtab entries, >>>> +# hence create an alternative System.map with namespace patched out to use for >>>> +# depmod. I.e. transform entries as follows: >>>> +# __ksymtab_NAMESPACE.symbol_name -> __ksymtab_symbol_name >>>> +sed 's/__ksymtab_.*\./__ksymtab_/' System.map > System.map.no_namespaces >>> >>>So people with old kmod will have to know they need to pass >>>System.map.no_namespaces rather than the usual >>>System.map. Also, distros will need to be update to also copy the new >>>file to the kernel package (or upgrade/patch kmod). >>> >>>I'd rather maintain the current format and fix the bug that patch is >>>fixing. The namespace >>>in the end IMO is just a small annoyance with a reason to exist. >> >>I agree, this fix is bad. >>We should not bother kmod or any tools. >>And System.map.no_namespaces is a cheesy workaround. > >Thanks for the feedback! > >Based on the discussion from last year about changing kmod/depmod for >this change to the ksymtab entries, >(https://lore.kernel.org/lkml/CAKi4VA+wvVxPxVSP6ruSR++TYvavKPNAQ5XhREF_9ZxeOKQirg@mail.gmail.com/), >I assumed this approach would be acceptable. The workaround (yeah, it is >a hack) is a way to mitigate the issue for people that can't update >depmod so easily. System.map.no_namespaces was not intended to be part >of any distribution, but rather used locally in depmod.sh. I could have >made this more clear. > >> >>BTW, I expressed my negative opinion in the review process >>for the patch set. I am still not convinced with the >>namespace feature, but anyway it was merged >>(with poor review and test). >> >> >> >>Get back on track, probably the right fix would be to >>stop using __ksymtab_<namespace>.<symbol>. >> >>It is not used for any purposes but passing >><namespace> / <symbol> pairs to modpost. >> >> >>For example, __kstrtabns_##sym points to >>the namespace string, so it would be possible >>to parse it from modpost? >> >>Then, asm("__ksymtab_" #ns NS_SEPARATOR #sym) >>will go away. > >I will give this a try. That sounds like a good idea. Maybe one way of going about it is to take the symbol value of __kstrtabns_##sym, subtract the section address (the section referred to by the st_shndx) from it to obtain the offset into __ksymtab_strings, and read in the namespace string from that offset.
diff --git a/.gitignore b/.gitignore index 70580bdd352c..5ed58a7cb433 100644 --- a/.gitignore +++ b/.gitignore @@ -59,6 +59,7 @@ modules.order /vmlinux-gdb.py /vmlinuz /System.map +/System.map.no_namespaces /Module.markers /modules.builtin.modinfo diff --git a/scripts/depmod.sh b/scripts/depmod.sh index e083bcae343f..602e1af072c7 100755 --- a/scripts/depmod.sh +++ b/scripts/depmod.sh @@ -39,7 +39,13 @@ if $depmod_hack_needed; then KERNELRELEASE=99.98.$KERNELRELEASE fi -set -- -ae -F System.map +# Older versions of depmod do not support symbol namespaces in ksymtab entries, +# hence create an alternative System.map with namespace patched out to use for +# depmod. I.e. transform entries as follows: +# __ksymtab_NAMESPACE.symbol_name -> __ksymtab_symbol_name +sed 's/__ksymtab_.*\./__ksymtab_/' System.map > System.map.no_namespaces + +set -- -ae -F System.map.no_namespaces if test -n "$INSTALL_MOD_PATH"; then set -- "$@" -b "$INSTALL_MOD_PATH" fi
depmod in its current version is not aware of symbol namespace in ksymtab entries introduced with 8651ec01daed ("module: add support for symbol namespaces."). They have the form __ksymtab_NAMESPACE.symbol_name A fix for kmod's depmod has been proposed [1]. In order to support older versions of depmod as well, create a System.map.no_namespaces during scripts/depmod.sh that has the pre-namespaces format. That way users do not immediately upgrade the userspace tool. [1] https://lore.kernel.org/linux-modules/20191004094136.166621-1-maennich@google.com/ Reported-by: Stefan Wahren <stefan.wahren@i2se.com> Fixes: 8651ec01daed ("module: add support for symbol namespaces.") Cc: Masahiro Yamada <yamada.masahiro@socionext.com> Cc: Lucas De Marchi <lucas.de.marchi@gmail.com> Cc: Jessica Yu <jeyu@kernel.org> Cc: Martijn Coenen <maco@android.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: linux-modules@vger.kernel.org Signed-off-by: Matthias Maennich <maennich@google.com> --- Please note this depends on the new ksymtab entry format proposed in https://lore.kernel.org/lkml/20191003075826.7478-2-yamada.masahiro@socionext.com/ That is likely to be merged soon as well as it fixes problems in 5.4-rc*, hence this patch depends on it. Cheers, Matthias .gitignore | 1 + scripts/depmod.sh | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-)