[PATCH v10 0/5] Kernel parameter parser cleanup/enhancement

[PATCH v10 0/5] Kernel parameter parser cleanup/enhancement

[Michal Suchanek]

Hello,

due to work on the fadump_extra_args I looked at the kernel parameter parser
and found its grammar rather curious.

It supports double quotes but not any other quoting characters so double quotes
cannot be quoted. What's more, the quotes can be anywhere in the parameter
name or value and are interpteted but are removed only from start and end of
the parameter value.

These are the patches not specific to fadump which somewhat straighten the
qouting grammar to make it on par with common shell interpreters.

Specifically double and single quotes can be used for quoting as well as
backslashes with the usual shell semantic. All quoting characters are removed
while the parameters are parsed.

Previous versions (including the fadump part) can be found here:

https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg126148.html
https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg123639.html

Thanks

Michal

Michal Suchanek (5):
  lib/cmdline.c: Add backslash support to kernel commandline parsing.
  Documentation/admin-guide: backslash support in kernel arguments.
  init/main.c: simplify repair_env_string.
  lib/cmdline.c: Implement single quotes in commandline argument
    parsing.
  Documentation/admin-guide: single quotes in kernel arguments.

 Documentation/admin-guide/kernel-parameters.rst |  5 +-
 init/main.c                                     | 13 ++---
 lib/cmdline.c                                   | 63 +++++++++++++++----------
 3 files changed, 46 insertions(+), 35 deletions(-)

-- 
2.13.6

[PATCH v10 1/5] lib/cmdline.c: Add backslash support to kernel commandline parsing.

[Michal Suchanek]

 - add \ as quoting character in kernel parameters
 - generalize quoting character removal to be able to remove quoting
 character at any position in kernel parameter rather than start and end
 only

This allows passing quotes in kernel arguments.

It is also useful to have quoting grammar more similar to shells and
bootloaders.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
- more verbose commit message
- optimize final quote removal
---
 lib/cmdline.c | 50 ++++++++++++++++++++++++++------------------------
 1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/lib/cmdline.c b/lib/cmdline.c
index 192848d880ea..aa0086dbb082 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -191,32 +191,43 @@ bool parse_option_str(const char *str, const char *option)
 	return false;
 }
 
+#define break_arg_end(i) { \
+	if (isspace(args[i]) && !in_quote && !backslash) \
+		break; \
+	}
+
 /*
  * Parse a string to get a param value pair.
- * You can use " around spaces, but can't escape ".
+ * You can use " around spaces, and you can escape with \
  * Hyphens and underscores equivalent in parameter names.
  */
 char *next_arg(char *args, char **param, char **val, int *state_flags)
 {
 	unsigned int i, equals = 0;
-	int in_quote = 0, quoted = 0;
+	int in_quote = 0, backslash = 0;
 	char *next;
 
-	if (*args == '"') {
-		args++;
-		in_quote = 1;
-		quoted = 1;
-	}
-
 	for (i = 0; args[i]; i++) {
-		if (isspace(args[i]) && !in_quote)
-			break;
-		if (equals == 0) {
-			if (args[i] == '=')
-				equals = i;
+		break_arg_end(i);
+
+		if ((equals == 0) && (args[i] == '='))
+			equals = i;
+
+		if (!backslash) {
+			if ((args[i] == '"') || (args[i] == '\\')) {
+				if (args[i] == '"')
+					in_quote = !in_quote;
+				if (args[i] == '\\')
+					backslash = 1;
+
+				break_arg_end(i + 1);
+				memmove(args + 1, args, i);
+				args++;
+				i--;
+			}
+		} else {
+			backslash = 0;
 		}
-		if (args[i] == '"')
-			in_quote = !in_quote;
 	}
 
 	if (state_flags)
@@ -231,16 +242,7 @@ char *next_arg(char *args, char **param, char **val, int *state_flags)
 	else {
 		args[equals] = '\0';
 		*val = args + equals + 1;
-
-		/* Don't include quotes in value. */
-		if (**val == '"') {
-			(*val)++;
-			if (args[i-1] == '"')
-				args[i-1] = '\0';
-		}
 	}
-	if (quoted && args[i-1] == '"')
-		args[i-1] = '\0';
 
 	if (args[i]) {
 		args[i] = '\0';
-- 
2.13.6

[PATCH v10 2/5] Documentation/admin-guide: backslash support in kernel arguments.

[Michal Suchanek]

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 Documentation/admin-guide/kernel-parameters.rst | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst
index b8d0bc07ed0a..327e76ebe37d 100644
--- a/Documentation/admin-guide/kernel-parameters.rst
+++ b/Documentation/admin-guide/kernel-parameters.rst
@@ -35,9 +35,9 @@ can also be entered as::
 
 	log-buf-len=1M print_fatal_signals=1
 
-Double-quotes can be used to protect spaces in values, e.g.::
+Double-quotes and backslashes can be used to protect spaces in values, e.g.::
 
-	param="spaces in here"
+	param="spaces in here" param2=spaces\ in\ here
 
 cpu lists:
 ----------
-- 
2.13.6

[PATCH v10 3/5] init/main.c: simplify repair_env_string.

[Michal Suchanek]

Quoting characters are now removed from the parameter so value always
follows directly after the NUL terminating parameter name.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 init/main.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/init/main.c b/init/main.c
index 4bc2819d97ad..d0f4a7a0f010 100644
--- a/init/main.c
+++ b/init/main.c
@@ -251,15 +251,10 @@ static int __init repair_env_string(char *param, char *val,
 				    const char *unused, void *arg)
 {
 	if (val) {
-		/* param=val or param="val"? */
-		if (val == param+strlen(param)+1)
-			val[-1] = '=';
-		else if (val == param+strlen(param)+2) {
-			val[-2] = '=';
-			memmove(val-1, val, strlen(val)+1);
-			val--;
-		} else
-			BUG();
+		int parm_len = strlen(param);
+
+		param[parm_len] = '=';
+		BUG_ON(val != param + parm_len + 1);
 	}
 	return 0;
 }
-- 
2.13.6

[PATCH v10 4/5] lib/cmdline.c: Implement single quotes in commandline argument parsing.

[Michal Suchanek]

This brings the kernel parser about on par with bourne shell, grub, and
other tools that chew the arguments before kernel does.

This should make it easier to deal with multiple levels of
nesting/quoting. With same quoting grammar on each level there is less
room for confusion.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
- optimize end quote removal
---
 lib/cmdline.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/lib/cmdline.c b/lib/cmdline.c
index aa0086dbb082..e699ed5aac8a 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -192,19 +192,26 @@ bool parse_option_str(const char *str, const char *option)
 }
 
 #define break_arg_end(i) { \
-	if (isspace(args[i]) && !in_quote && !backslash) \
+	if (isspace(args[i]) && !in_quote && !backslash && !in_single) \
 		break; \
 	}
 
+#define squash_char { \
+	break_arg_end(i + 1); \
+	memmove(args + 1, args, i); \
+	args++; \
+	i--; \
+}
+
 /*
  * Parse a string to get a param value pair.
- * You can use " around spaces, and you can escape with \
+ * You can use " or ' around spaces, and you can escape with \
  * Hyphens and underscores equivalent in parameter names.
  */
 char *next_arg(char *args, char **param, char **val, int *state_flags)
 {
 	unsigned int i, equals = 0;
-	int in_quote = 0, backslash = 0;
+	int in_quote = 0, backslash = 0, in_single = 0;
 	char *next;
 
 	for (i = 0; args[i]; i++) {
@@ -213,17 +220,23 @@ char *next_arg(char *args, char **param, char **val, int *state_flags)
 		if ((equals == 0) && (args[i] == '='))
 			equals = i;
 
-		if (!backslash) {
-			if ((args[i] == '"') || (args[i] == '\\')) {
+		if (in_single) {
+			if (args[i] == '\'') {
+				in_single = 0;
+
+				squash_char;
+			}
+		} else if (!backslash) {
+			if ((args[i] == '"') || (args[i] == '\\') ||
+					(args[i] == '\'')) {
 				if (args[i] == '"')
 					in_quote = !in_quote;
 				if (args[i] == '\\')
 					backslash = 1;
+				if (args[i] == '\'')
+					in_single = 1;
 
-				break_arg_end(i + 1);
-				memmove(args + 1, args, i);
-				args++;
-				i--;
+				squash_char;
 			}
 		} else {
 			backslash = 0;
-- 
2.13.6

[PATCH v10 5/5] Documentation/admin-guide: single quotes in kernel arguments.

[Michal Suchanek]

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
- fix typo
---
 Documentation/admin-guide/kernel-parameters.rst | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst
index 327e76ebe37d..cc81ad7cab0c 100644
--- a/Documentation/admin-guide/kernel-parameters.rst
+++ b/Documentation/admin-guide/kernel-parameters.rst
@@ -35,9 +35,10 @@ can also be entered as::
 
 	log-buf-len=1M print_fatal_signals=1
 
-Double-quotes and backslashes can be used to protect spaces in values, e.g.::
+Double-quotes, single-quotes and backslashes can be used to protect spaces
+in values, e.g.::
 
-	param="spaces in here" param2=spaces\ in\ here
+	param="spaces in here" param2=spaces\ in\ here param3='@%# !\'
 
 cpu lists:
 ----------
-- 
2.13.6

Re: [PATCH v10 0/5] Kernel parameter parser cleanup/enhancement

[Andy Shevchenko]

On Tue, Jun 5, 2018 at 7:43 PM, Michal Suchanek <msuchanek@suse.de> wrote:
> Hello,
>
> due to work on the fadump_extra_args I looked at the kernel parameter parser
> and found its grammar rather curious.
>
> It supports double quotes but not any other quoting characters so double quotes
> cannot be quoted. What's more, the quotes can be anywhere in the parameter
> name or value and are interpteted but are removed only from start and end of
> the parameter value.
>
> These are the patches not specific to fadump which somewhat straighten the
> qouting grammar to make it on par with common shell interpreters.

I didn't notice any use of string_unescape_*() functionality. So, your
patch is kinda very specific to some narrow subset of escaping and
unescaping stuff.
Thus, it's still not on par with shell, right?

>
> Specifically double and single quotes can be used for quoting as well as
> backslashes with the usual shell semantic. All quoting characters are removed
> while the parameters are parsed.
>
> Previous versions (including the fadump part) can be found here:
>
> https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg126148.html
> https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg123639.html
>
> Thanks
>
> Michal
>
> Michal Suchanek (5):
>   lib/cmdline.c: Add backslash support to kernel commandline parsing.
>   Documentation/admin-guide: backslash support in kernel arguments.
>   init/main.c: simplify repair_env_string.
>   lib/cmdline.c: Implement single quotes in commandline argument
>     parsing.
>   Documentation/admin-guide: single quotes in kernel arguments.
>
>  Documentation/admin-guide/kernel-parameters.rst |  5 +-
>  init/main.c                                     | 13 ++---
>  lib/cmdline.c                                   | 63 +++++++++++++++----------
>  3 files changed, 46 insertions(+), 35 deletions(-)
>
> --
> 2.13.6
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v10 0/5] Kernel parameter parser cleanup/enhancement

[Michal Suchánek]

On Tue, 5 Jun 2018 20:05:50 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Jun 5, 2018 at 7:43 PM, Michal Suchanek <msuchanek@suse.de>
> wrote:
> > Hello,
> >
> > due to work on the fadump_extra_args I looked at the kernel
> > parameter parser and found its grammar rather curious.
> >
> > It supports double quotes but not any other quoting characters so
> > double quotes cannot be quoted. What's more, the quotes can be
> > anywhere in the parameter name or value and are interpteted but are
> > removed only from start and end of the parameter value.
> >
> > These are the patches not specific to fadump which somewhat
> > straighten the qouting grammar to make it on par with common shell
> > interpreters.  
> 
> I didn't notice any use of string_unescape_*() functionality. So, your
> patch is kinda very specific to some narrow subset of escaping and
> unescaping stuff.

It does what it says. It cannot use string_unescape because it needs to
determine the boundaries of quoted strings.

> Thus, it's still not on par with shell, right?

It does not interpret special character sequences other than quoting.

The feature would not be hard to add but I do not see the need.

Unfortunately, the existing string_unescape is totally not fitting to
be integrated into the parser for the purpose.

Thanks

Michal

> 
> >
> > Specifically double and single quotes can be used for quoting as
> > well as backslashes with the usual shell semantic. All quoting
> > characters are removed while the parameters are parsed.
> >

Re: [PATCH v10 0/5] Kernel parameter parser cleanup/enhancement

[Andrew Morton]

On Tue,  5 Jun 2018 18:43:07 +0200 Michal Suchanek <msuchanek@suse.de> wrote:

> due to work on the fadump_extra_args I looked at the kernel parameter parser
> and found its grammar rather curious.
> 
> It supports double quotes but not any other quoting characters so double quotes
> cannot be quoted. What's more, the quotes can be anywhere in the parameter
> name or value and are interpteted but are removed only from start and end of
> the parameter value.
> 
> These are the patches not specific to fadump which somewhat straighten the
> qouting grammar to make it on par with common shell interpreters.
> 
> Specifically double and single quotes can be used for quoting as well as
> backslashes with the usual shell semantic. All quoting characters are removed
> while the parameters are parsed.

Well.  It's nice.  I guess.  Is there any demand for these
capabilities?  I don't recall ever having seen a complaint - kernel
parameters tend to be pretty simple things.


Also, the break_arg_end() and squash_char() macros make me want to cry.
A macro which changes control flow hidden inside another macro!  Are
they reeeealy necessary?  Can't be done with some C helpers?  Maybe put
inquote, backslash, args, i into a new struct parser_state and pass a
pointer to that around the place?  At the very least, those macros
should be apologetically documented :(

Re: [PATCH v10 0/5] Kernel parameter parser cleanup/enhancement

[Michal Suchánek]

On Tue, 19 Jun 2018 16:36:47 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue,  5 Jun 2018 18:43:07 +0200 Michal Suchanek
> <msuchanek@suse.de> wrote:
> 
> > due to work on the fadump_extra_args I looked at the kernel
> > parameter parser and found its grammar rather curious.
> > 
> > It supports double quotes but not any other quoting characters so
> > double quotes cannot be quoted. What's more, the quotes can be
> > anywhere in the parameter name or value and are interpteted but are
> > removed only from start and end of the parameter value.
> > 
> > These are the patches not specific to fadump which somewhat
> > straighten the qouting grammar to make it on par with common shell
> > interpreters.
> > 
> > Specifically double and single quotes can be used for quoting as
> > well as backslashes with the usual shell semantic. All quoting
> > characters are removed while the parameters are parsed.  
> 
> Well.  It's nice.  I guess.  Is there any demand for these
> capabilities?  I don't recall ever having seen a complaint - kernel
> parameters tend to be pretty simple things.

Yes, the complaint came with the nested arguments which are now not
pursued anymore. The grammar is really not very nice as it is, though.

> Also, the break_arg_end() and squash_char() macros make me want to
> cry. A macro which changes control flow hidden inside another macro!
> Are they reeeealy necessary?  

Seems better than repeating the same code 3 times.

> Can't be done with some C helpers?

You could not change the control flow then, could you?

Technically you could return something and decide based on that I
suppose.

> Maybe put inquote, backslash, args, i into a new struct parser_state
> and pass a pointer to that around the place?  At the very least,
> those macros should be apologetically documented :(

Yes, some description can be added, too.

Thanks

Michal