depmod: account for new namespace field in Module.symvers (for kernel versions >= 5.4)
diff mbox series

Message ID 20200221165243.25100-1-jeyu@kernel.org
State New
Headers show
Series
  • depmod: account for new namespace field in Module.symvers (for kernel versions >= 5.4)
Related show

Commit Message

Jessica Yu Feb. 21, 2020, 4:52 p.m. UTC
depmod -e -E is broken for kernel versions >= 5.4, because a new
namespace field was added to Module.symvers. Each line is tab delimited
with 5 fields in total. E.g.,

	0xb352177e\tfind_first_bit\tnamespace\tvmlinux\tEXPORT_SYMBOL

When a symbol doesn't have a namespace, then the namespace field is empty:

	0xb352177e\tfind_first_bit\t\tvmlinux\tEXPORT_SYMBOL

Fix up depmod_load_symvers() so it finds the crc, symbol, and module
("where") fields correctly. Since there can be empty fields, strsep() is
used instead of strtok(), since strtok() cannot handle empty fields
(i.e., two delimiters in succession).

Signed-off-by: Jessica Yu <jeyu@kernel.org>
---

Hi,

I was not sure what the best way of determining the symvers format was. I
guess counting delimiters isn't the prettiest way, but if anyone has a
better idea, let me know. Thanks!

 tools/depmod.c | 35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

Comments

Jessica Yu March 3, 2020, 2:31 p.m. UTC | #1
+++ Jessica Yu [21/02/20 17:52 +0100]:
>depmod -e -E is broken for kernel versions >= 5.4, because a new
>namespace field was added to Module.symvers. Each line is tab delimited
>with 5 fields in total. E.g.,
>
>	0xb352177e\tfind_first_bit\tnamespace\tvmlinux\tEXPORT_SYMBOL
>
>When a symbol doesn't have a namespace, then the namespace field is empty:
>
>	0xb352177e\tfind_first_bit\t\tvmlinux\tEXPORT_SYMBOL
>
>Fix up depmod_load_symvers() so it finds the crc, symbol, and module
>("where") fields correctly. Since there can be empty fields, strsep() is
>used instead of strtok(), since strtok() cannot handle empty fields
>(i.e., two delimiters in succession).
>
>Signed-off-by: Jessica Yu <jeyu@kernel.org>

Friendly ping? :-)

Thanks,

Jessica
Lucas De Marchi March 4, 2020, 6:28 a.m. UTC | #2
On Fri, Feb 21, 2020 at 8:53 AM Jessica Yu <jeyu@kernel.org> wrote:
>
> depmod -e -E is broken for kernel versions >= 5.4, because a new
> namespace field was added to Module.symvers. Each line is tab delimited
> with 5 fields in total. E.g.,
>
>         0xb352177e\tfind_first_bit\tnamespace\tvmlinux\tEXPORT_SYMBOL
>
> When a symbol doesn't have a namespace, then the namespace field is empty:
>
>         0xb352177e\tfind_first_bit\t\tvmlinux\tEXPORT_SYMBOL

Why is namespace added in the *middle*? We remember we specifically
talked about compatibility back when this was added. Why is it not at
the end so tools that don't know about namespace don't stop working?

Lucas De Marchi

>
> Fix up depmod_load_symvers() so it finds the crc, symbol, and module
> ("where") fields correctly. Since there can be empty fields, strsep() is
> used instead of strtok(), since strtok() cannot handle empty fields
> (i.e., two delimiters in succession).
>
> Signed-off-by: Jessica Yu <jeyu@kernel.org>
> ---
>
> Hi,
>
> I was not sure what the best way of determining the symvers format was. I
> guess counting delimiters isn't the prettiest way, but if anyone has a
> better idea, let me know. Thanks!
>
>  tools/depmod.c | 35 ++++++++++++++++++++++++++++++-----
>  1 file changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/tools/depmod.c b/tools/depmod.c
> index fbbce10..c5b9612 100644
> --- a/tools/depmod.c
> +++ b/tools/depmod.c
> @@ -2577,7 +2577,9 @@ static int depmod_load_symvers(struct depmod *depmod, const char *filename)
>  {
>         char line[10240];
>         FILE *fp;
> -       unsigned int linenum = 0;
> +       unsigned int linenum = 0, cnt = 0;
> +       bool uses_namespaces = false;
> +       char *ptr;
>
>         fp = fopen(filename, "r");
>         if (fp == NULL) {
> @@ -2587,7 +2589,26 @@ static int depmod_load_symvers(struct depmod *depmod, const char *filename)
>         }
>         DBG("load symvers: %s\n", filename);
>
> -       /* eg. "0xb352177e\tfind_first_bit\tvmlinux\tEXPORT_SYMBOL" */
> +       /*
> +        * First, check for >=5.4 Module.symvers format:
> +        * "0xb352177e\tfind_first_bit\tnamespace\tvmlinux\tEXPORT_SYMBOL"
> +        * If there are 5 fields (4 tabs), assume we're using the new format.
> +        */
> +       fgets(line, sizeof(line), fp);
> +       ptr = line;
> +       while ((ptr = strchr(ptr, '\t')) != NULL) {
> +               cnt++;
> +               ptr++;
> +       }
> +       if (cnt > 3)
> +               uses_namespaces = true;
> +       rewind(fp);
> +
> +       /*
> +        * eg. "0xb352177e\tfind_first_bit\tvmlinux\tEXPORT_SYMBOL"
> +        * Or if kernel version >=5.4:
> +        * "0xb352177e\tfind_first_bit\tnamespace\tvmlinux\tEXPORT_SYMBOL"
> +        */
>         while (fgets(line, sizeof(line), fp) != NULL) {
>                 const char *ver, *sym, *where;
>                 char *verend;
> @@ -2595,9 +2616,13 @@ static int depmod_load_symvers(struct depmod *depmod, const char *filename)
>
>                 linenum++;
>
> -               ver = strtok(line, " \t");
> -               sym = strtok(NULL, " \t");
> -               where = strtok(NULL, " \t");
> +               ptr = (char *)line;
> +               ver = strsep(&ptr, "\t");
> +               sym = strsep(&ptr, "\t");
> +               if (uses_namespaces) /* skip namespace field */
> +                       strsep(&ptr, "\t");
> +               where = strsep(&ptr, "\t");
> +
>                 if (!ver || !sym || !where)
>                         continue;
>
> --
> 2.16.4
>
Jessica Yu March 4, 2020, 9:18 a.m. UTC | #3
+++ Lucas De Marchi [03/03/20 22:28 -0800]:
>On Fri, Feb 21, 2020 at 8:53 AM Jessica Yu <jeyu@kernel.org> wrote:
>>
>> depmod -e -E is broken for kernel versions >= 5.4, because a new
>> namespace field was added to Module.symvers. Each line is tab delimited
>> with 5 fields in total. E.g.,
>>
>>         0xb352177e\tfind_first_bit\tnamespace\tvmlinux\tEXPORT_SYMBOL
>>
>> When a symbol doesn't have a namespace, then the namespace field is empty:
>>
>>         0xb352177e\tfind_first_bit\t\tvmlinux\tEXPORT_SYMBOL
>
>Why is namespace added in the *middle*? We remember we specifically
>talked about compatibility back when this was added. Why is it not at
>the end so tools that don't know about namespace don't stop working?
>
>Lucas De Marchi

Sigh, I do remember we had a short discussion upstream back in August
[1] when we were tossing around Module.symvers format preferences. It
is my fault for not having pushed the backwards compatibility option
more instead of thinking it could be patched up in kmod tools. I think
maybe the best course of option is to revert this upstream instead and
Cc:stable.

Sorry about this. :-/

[1] https://lore.kernel.org/r/20190828094325.GA25048@linux-8ccs
Matthias Maennich March 4, 2020, 11:34 a.m. UTC | #4
On Wed, Mar 04, 2020 at 10:18:33AM +0100, Jessica Yu wrote:
>+++ Lucas De Marchi [03/03/20 22:28 -0800]:
>>On Fri, Feb 21, 2020 at 8:53 AM Jessica Yu <jeyu@kernel.org> wrote:
>>>
>>>depmod -e -E is broken for kernel versions >= 5.4, because a new
>>>namespace field was added to Module.symvers. Each line is tab delimited
>>>with 5 fields in total. E.g.,
>>>
>>>        0xb352177e\tfind_first_bit\tnamespace\tvmlinux\tEXPORT_SYMBOL
>>>
>>>When a symbol doesn't have a namespace, then the namespace field is empty:
>>>
>>>        0xb352177e\tfind_first_bit\t\tvmlinux\tEXPORT_SYMBOL
>>
>>Why is namespace added in the *middle*? We remember we specifically
>>talked about compatibility back when this was added. Why is it not at
>>the end so tools that don't know about namespace don't stop working?
>>
>>Lucas De Marchi
>
>Sigh, I do remember we had a short discussion upstream back in August
>[1] when we were tossing around Module.symvers format preferences. It
>is my fault for not having pushed the backwards compatibility option
>more instead of thinking it could be patched up in kmod tools. I think
>maybe the best course of option is to revert this upstream instead and
>Cc:stable.
>
>Sorry about this. :-/

Sorry, my fault. The discussion went first all around whether we should
append the namespace to the symbol name. This we did not do.
Then we discussed whether the last values of this line are actually
optional and we settled with the format now merged as nobody further
objected in the tail end of this discussion:
  https://lore.kernel.org/linux-modules/20190828101640.GB25048@linux-8ccs/

We could probably move the namespace to the end as the other fields are
not optional (at least judging from write_dump() in modpost.c).

Cheers,
Matthias

>
>[1] https://lore.kernel.org/r/20190828094325.GA25048@linux-8ccs
>
Lucas De Marchi March 6, 2020, 12:10 a.m. UTC | #5
On Wed, Mar 04, 2020 at 10:18:33AM +0100, Jessica Yu wrote:
>+++ Lucas De Marchi [03/03/20 22:28 -0800]:
>>On Fri, Feb 21, 2020 at 8:53 AM Jessica Yu <jeyu@kernel.org> wrote:
>>>
>>>depmod -e -E is broken for kernel versions >= 5.4, because a new
>>>namespace field was added to Module.symvers. Each line is tab delimited
>>>with 5 fields in total. E.g.,
>>>
>>>        0xb352177e\tfind_first_bit\tnamespace\tvmlinux\tEXPORT_SYMBOL
>>>
>>>When a symbol doesn't have a namespace, then the namespace field is empty:
>>>
>>>        0xb352177e\tfind_first_bit\t\tvmlinux\tEXPORT_SYMBOL
>>
>>Why is namespace added in the *middle*? We remember we specifically
>>talked about compatibility back when this was added. Why is it not at
>>the end so tools that don't know about namespace don't stop working?
>>
>>Lucas De Marchi
>
>Sigh, I do remember we had a short discussion upstream back in August
>[1] when we were tossing around Module.symvers format preferences. It
>is my fault for not having pushed the backwards compatibility option
>more instead of thinking it could be patched up in kmod tools. I think
>maybe the best course of option is to revert this upstream instead and
>Cc:stable.

Yeah I didn't follow that series thoroughly as I should. I agree that
the best course of action now is to update the format and CC stable.

Lucas De Marchi

>
>Sorry about this. :-/
>
>[1] https://lore.kernel.org/r/20190828094325.GA25048@linux-8ccs
>

Patch
diff mbox series

diff --git a/tools/depmod.c b/tools/depmod.c
index fbbce10..c5b9612 100644
--- a/tools/depmod.c
+++ b/tools/depmod.c
@@ -2577,7 +2577,9 @@  static int depmod_load_symvers(struct depmod *depmod, const char *filename)
 {
 	char line[10240];
 	FILE *fp;
-	unsigned int linenum = 0;
+	unsigned int linenum = 0, cnt = 0;
+	bool uses_namespaces = false;
+	char *ptr;
 
 	fp = fopen(filename, "r");
 	if (fp == NULL) {
@@ -2587,7 +2589,26 @@  static int depmod_load_symvers(struct depmod *depmod, const char *filename)
 	}
 	DBG("load symvers: %s\n", filename);
 
-	/* eg. "0xb352177e\tfind_first_bit\tvmlinux\tEXPORT_SYMBOL" */
+	/*
+	 * First, check for >=5.4 Module.symvers format:
+	 * "0xb352177e\tfind_first_bit\tnamespace\tvmlinux\tEXPORT_SYMBOL"
+	 * If there are 5 fields (4 tabs), assume we're using the new format.
+	 */
+	fgets(line, sizeof(line), fp);
+	ptr = line;
+	while ((ptr = strchr(ptr, '\t')) != NULL) {
+		cnt++;
+		ptr++;
+	}
+	if (cnt > 3)
+		uses_namespaces = true;
+	rewind(fp);
+
+	/*
+	 * eg. "0xb352177e\tfind_first_bit\tvmlinux\tEXPORT_SYMBOL"
+	 * Or if kernel version >=5.4:
+	 * "0xb352177e\tfind_first_bit\tnamespace\tvmlinux\tEXPORT_SYMBOL"
+	 */
 	while (fgets(line, sizeof(line), fp) != NULL) {
 		const char *ver, *sym, *where;
 		char *verend;
@@ -2595,9 +2616,13 @@  static int depmod_load_symvers(struct depmod *depmod, const char *filename)
 
 		linenum++;
 
-		ver = strtok(line, " \t");
-		sym = strtok(NULL, " \t");
-		where = strtok(NULL, " \t");
+		ptr = (char *)line;
+		ver = strsep(&ptr, "\t");
+		sym = strsep(&ptr, "\t");
+		if (uses_namespaces) /* skip namespace field */
+			strsep(&ptr, "\t");
+		where = strsep(&ptr, "\t");
+
 		if (!ver || !sym || !where)
 			continue;