linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RESENT] coredump: fix cn_printf formatting warnings
@ 2015-04-16 10:28 Nicolas Iooss
  2015-04-16 10:50 ` Joe Perches
  0 siblings, 1 reply; 10+ messages in thread
From: Nicolas Iooss @ 2015-04-16 10:28 UTC (permalink / raw)
  To: viro; +Cc: linux-fsdevel, linux-kernel, Nicolas Iooss

Add __printf attributes to cn_*printf functions.  With these, gcc says:

  fs/coredump.c:213:5: warning: format '%d' expects argument of type
  'int', but argument 3 has type 'kuid_t' [-Wformat=]
       err = cn_printf(cn, "%d", cred->uid);
       ^
  fs/coredump.c:217:5: warning: format '%d' expects argument of type
  'int', but argument 3 has type 'kgid_t' [-Wformat=]
       err = cn_printf(cn, "%d", cred->gid);
       ^
  fs/coredump.c:225:5: warning: format '%ld' expects argument of type
  'long int', but argument 3 has type 'int' [-Wformat=]
       err = cn_printf(cn, "%ld", cprm->siginfo->si_signo);
       ^

The third warning is easily fixed as si_signo is always an int.

For the two others, cred->uid and cred->gid need to be converted to
either a user-namespace UID/GID or to init_user_ns UID/GID.  As
Documentation/sysctl/kernel.txt does not specify which user namespace is
used to translate %u and %g in core_pattern, but lowercase %p and %i are
used to format pid/tid in current process namespace, it seems intuitive
that lowercase %u and %g use the current user namespace.  So implement
this.

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
---

I sent this patch more than a month ago but go no feedback, so I'm sending it again.
Comments would be greatly appreciated.

 fs/coredump.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index bbbe139ab280..977fc8b91f2d 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -70,7 +70,8 @@ static int expand_corename(struct core_name *cn, int size)
 	return 0;
 }
 
-static int cn_vprintf(struct core_name *cn, const char *fmt, va_list arg)
+static __printf(2, 0) int cn_vprintf(struct core_name *cn, const char *fmt,
+				     va_list arg)
 {
 	int free, need;
 	va_list arg_copy;
@@ -93,7 +94,7 @@ again:
 	return -ENOMEM;
 }
 
-static int cn_printf(struct core_name *cn, const char *fmt, ...)
+static __printf(2, 3) int cn_printf(struct core_name *cn, const char *fmt, ...)
 {
 	va_list arg;
 	int ret;
@@ -105,7 +106,8 @@ static int cn_printf(struct core_name *cn, const char *fmt, ...)
 	return ret;
 }
 
-static int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
+static __printf(2, 3)
+int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
 {
 	int cur = cn->used;
 	va_list arg;
@@ -209,11 +211,15 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
 				break;
 			/* uid */
 			case 'u':
-				err = cn_printf(cn, "%d", cred->uid);
+				err = cn_printf(cn, "%d",
+						from_kuid_munged(cred->user_ns,
+								 cred->uid));
 				break;
 			/* gid */
 			case 'g':
-				err = cn_printf(cn, "%d", cred->gid);
+				err = cn_printf(cn, "%d",
+						from_kgid_munged(cred->user_ns,
+								 cred->gid));
 				break;
 			case 'd':
 				err = cn_printf(cn, "%d",
@@ -221,7 +227,7 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
 				break;
 			/* signal that caused the coredump */
 			case 's':
-				err = cn_printf(cn, "%ld", cprm->siginfo->si_signo);
+				err = cn_printf(cn, "%d", cprm->siginfo->si_signo);
 				break;
 			/* UNIX time of coredump */
 			case 't': {
-- 
2.3.5


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH RESENT] coredump: fix cn_printf formatting warnings
  2015-04-16 10:28 [PATCH RESENT] coredump: fix cn_printf formatting warnings Nicolas Iooss
@ 2015-04-16 10:50 ` Joe Perches
  2015-05-03 10:34   ` [PATCH 1/2] coredump: use from_kuid/kgid_munged when formatting corename Nicolas Iooss
  0 siblings, 1 reply; 10+ messages in thread
From: Joe Perches @ 2015-04-16 10:50 UTC (permalink / raw)
  To: Nicolas Iooss; +Cc: viro, linux-fsdevel, linux-kernel, Andrew Morton

(adding Andrew Morton to cc's)

On Thu, 2015-04-16 at 18:28 +0800, Nicolas Iooss wrote:
> Add __printf attributes to cn_*printf functions.

[]

> I sent this patch more than a month ago but go no feedback, so I'm sending it again.
> Comments would be greatly appreciated.

Seems sensible, but trivially, the uid/gid output
should use %u as uid_t/__kernel_uid32_t and
gid_t/__kernel_gid32_t are unsigned int

Also, this would probably be better as 2 patches.

One for the __printf and mismatches fixes,
Another for the _munged use.

> diff --git a/fs/coredump.c b/fs/coredump.c
[]
> @@ -209,11 +211,15 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
>  				break;
>  			/* uid */
>  			case 'u':
> -				err = cn_printf(cn, "%d", cred->uid);
> +				err = cn_printf(cn, "%d",
> +						from_kuid_munged(cred->user_ns,
> +								 cred->uid));
>  				break;
>  			/* gid */
>  			case 'g':
> -				err = cn_printf(cn, "%d", cred->gid);
> +				err = cn_printf(cn, "%d",
> +						from_kgid_munged(cred->user_ns,
> +								 cred->gid));
>  				break;
>  			case 'd':
>  				err = cn_printf(cn, "%d",

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] coredump: use from_kuid/kgid_munged when formatting corename
  2015-04-16 10:50 ` Joe Perches
@ 2015-05-03 10:34   ` Nicolas Iooss
  2015-05-03 10:34     ` [PATCH 2/2] coredump: add __printf attribute to cn_*printf functions Nicolas Iooss
       [not found]     ` <1430649246-32726-1-git-send-email-nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
  0 siblings, 2 replies; 10+ messages in thread
From: Nicolas Iooss @ 2015-05-03 10:34 UTC (permalink / raw)
  To: Alexander Viro, linux-fsdevel; +Cc: Andrew Morton, linux-kernel, Nicolas Iooss

When adding __printf attribute to cn_printf, gcc reports some issues:

  fs/coredump.c:213:5: warning: format '%d' expects argument of type
  'int', but argument 3 has type 'kuid_t' [-Wformat=]
       err = cn_printf(cn, "%d", cred->uid);
       ^
  fs/coredump.c:217:5: warning: format '%d' expects argument of type
  'int', but argument 3 has type 'kgid_t' [-Wformat=]
       err = cn_printf(cn, "%d", cred->gid);
       ^

These warnings come from the fact that the value of uid/gid needs to be
extracted from the kuid_t/kgid_t structure before being used as an
integer.  More precisely, cred->uid and cred->gid need to be converted
to either user-namespace uid/gid or to init_user_ns uid/gid.

As Documentation/sysctl/kernel.txt does not specify which user namespace
is used to translate %u and %g in core_pattern, but lowercase %p and %i
are used to format pid/tid in the current process namespace, it seems
intuitive that lowercase %u and %g use the current user namespace.

While at it, format uid and gid values with %u instead of %d because
uid_t/__kernel_uid32_t and gid_t/__kernel_gid32_t are unsigned int.

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
---
 fs/coredump.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index bbbe139ab280..99c8af640c5a 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -209,11 +209,15 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
 				break;
 			/* uid */
 			case 'u':
-				err = cn_printf(cn, "%d", cred->uid);
+				err = cn_printf(cn, "%u",
+						from_kuid_munged(cred->user_ns,
+								 cred->uid));
 				break;
 			/* gid */
 			case 'g':
-				err = cn_printf(cn, "%d", cred->gid);
+				err = cn_printf(cn, "%u",
+						from_kgid_munged(cred->user_ns,
+								 cred->gid));
 				break;
 			case 'd':
 				err = cn_printf(cn, "%d",
-- 
2.3.6


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] coredump: add __printf attribute to cn_*printf functions
  2015-05-03 10:34   ` [PATCH 1/2] coredump: use from_kuid/kgid_munged when formatting corename Nicolas Iooss
@ 2015-05-03 10:34     ` Nicolas Iooss
       [not found]     ` <1430649246-32726-1-git-send-email-nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
  1 sibling, 0 replies; 10+ messages in thread
From: Nicolas Iooss @ 2015-05-03 10:34 UTC (permalink / raw)
  To: Alexander Viro, linux-fsdevel; +Cc: Andrew Morton, linux-kernel, Nicolas Iooss

This allows detecting improper format string at build time, like:

  fs/coredump.c:225:5: warning: format '%ld' expects argument of type
  'long int', but argument 3 has type 'int' [-Wformat=]
       err = cn_printf(cn, "%ld", cprm->siginfo->si_signo);
       ^

As si_signo is always an int, the format should be %d here.

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
---
 fs/coredump.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index 99c8af640c5a..30fc1a83cb09 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -70,7 +70,8 @@ static int expand_corename(struct core_name *cn, int size)
 	return 0;
 }
 
-static int cn_vprintf(struct core_name *cn, const char *fmt, va_list arg)
+static __printf(2, 0) int cn_vprintf(struct core_name *cn, const char *fmt,
+				     va_list arg)
 {
 	int free, need;
 	va_list arg_copy;
@@ -93,7 +94,7 @@ again:
 	return -ENOMEM;
 }
 
-static int cn_printf(struct core_name *cn, const char *fmt, ...)
+static __printf(2, 3) int cn_printf(struct core_name *cn, const char *fmt, ...)
 {
 	va_list arg;
 	int ret;
@@ -105,7 +106,8 @@ static int cn_printf(struct core_name *cn, const char *fmt, ...)
 	return ret;
 }
 
-static int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
+static __printf(2, 3)
+int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
 {
 	int cur = cn->used;
 	va_list arg;
@@ -225,7 +227,7 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
 				break;
 			/* signal that caused the coredump */
 			case 's':
-				err = cn_printf(cn, "%ld", cprm->siginfo->si_signo);
+				err = cn_printf(cn, "%d", cprm->siginfo->si_signo);
 				break;
 			/* UNIX time of coredump */
 			case 't': {
-- 
2.3.6

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] coredump: use from_kuid/kgid_munged when formatting corename
       [not found]     ` <1430649246-32726-1-git-send-email-nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
@ 2015-05-05 19:13       ` Eric W. Biederman
       [not found]         ` <87bnhyhm7f.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
  0 siblings, 1 reply; 10+ messages in thread
From: Eric W. Biederman @ 2015-05-05 19:13 UTC (permalink / raw)
  To: Nicolas Iooss
  Cc: linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Linux Containers,
	Andrew Morton, Alexander Viro,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org> writes:

> When adding __printf attribute to cn_printf, gcc reports some issues:
>
>   fs/coredump.c:213:5: warning: format '%d' expects argument of type
>   'int', but argument 3 has type 'kuid_t' [-Wformat=]
>        err = cn_printf(cn, "%d", cred->uid);
>        ^
>   fs/coredump.c:217:5: warning: format '%d' expects argument of type
>   'int', but argument 3 has type 'kgid_t' [-Wformat=]
>        err = cn_printf(cn, "%d", cred->gid);
>        ^
>
> These warnings come from the fact that the value of uid/gid needs to be
> extracted from the kuid_t/kgid_t structure before being used as an
> integer.  More precisely, cred->uid and cred->gid need to be converted
> to either user-namespace uid/gid or to init_user_ns uid/gid.
>
> As Documentation/sysctl/kernel.txt does not specify which user namespace
> is used to translate %u and %g in core_pattern, but lowercase %p and %i
> are used to format pid/tid in the current process namespace, it seems
> intuitive that lowercase %u and %g use the current user namespace.

I love the logic of lower vs upper case letters in the selection of how
an identifier should be used.  Unfortunately I can't support it.

Converting to anything other than init_user_ns will actually be an ABI
break.  Which in practice should trump everything else.

Further only the global root user can set this value, which largely
implies that the program setting the core dump patter will expect the
values to be in the initial user namespace.

In practice your patch allows any user to put any uid they want on core
files which seems to make the uid parameter useless.

So for all of the reasons above I think we need to print uids and gids
in the initial user namespace unless explicitly requested to do so.

> While at it, format uid and gid values with %u instead of %d because
> uid_t/__kernel_uid32_t and gid_t/__kernel_gid32_t are unsigned int.
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
> ---
>  fs/coredump.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/coredump.c b/fs/coredump.c
> index bbbe139ab280..99c8af640c5a 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -209,11 +209,15 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
>  				break;
>  			/* uid */
>  			case 'u':
> -				err = cn_printf(cn, "%d", cred->uid);
> +				err = cn_printf(cn, "%u",
> +						from_kuid_munged(cred->user_ns,
> +								 cred->uid));
>  				break;
>  			/* gid */
>  			case 'g':
> -				err = cn_printf(cn, "%d", cred->gid);
> +				err = cn_printf(cn, "%u",
> +						from_kgid_munged(cred->user_ns,
> +								 cred->gid));
>  				break;
>  			case 'd':
>  				err = cn_printf(cn, "%d",

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] coredump: use from_kuid/kgid_munged when formatting corename
       [not found]         ` <87bnhyhm7f.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
@ 2015-05-06 12:18           ` Nicolas Iooss
  2015-05-15  2:29             ` [PATCH v2 1/2] coredump: use from_kuid/kgid " Nicolas Iooss
  0 siblings, 1 reply; 10+ messages in thread
From: Nicolas Iooss @ 2015-05-06 12:18 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Linux Containers,
	Andrew Morton, Alexander Viro,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

On 05/06/2015 03:13 AM, Eric W. Biederman wrote:
> Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org> writes:
> 
>> When adding __printf attribute to cn_printf, gcc reports some issues:
>>
>>   fs/coredump.c:213:5: warning: format '%d' expects argument of type
>>   'int', but argument 3 has type 'kuid_t' [-Wformat=]
>>        err = cn_printf(cn, "%d", cred->uid);
>>        ^
>>   fs/coredump.c:217:5: warning: format '%d' expects argument of type
>>   'int', but argument 3 has type 'kgid_t' [-Wformat=]
>>        err = cn_printf(cn, "%d", cred->gid);
>>        ^
>>
>> These warnings come from the fact that the value of uid/gid needs to be
>> extracted from the kuid_t/kgid_t structure before being used as an
>> integer.  More precisely, cred->uid and cred->gid need to be converted
>> to either user-namespace uid/gid or to init_user_ns uid/gid.
>>
>> As Documentation/sysctl/kernel.txt does not specify which user namespace
>> is used to translate %u and %g in core_pattern, but lowercase %p and %i
>> are used to format pid/tid in the current process namespace, it seems
>> intuitive that lowercase %u and %g use the current user namespace.
> 
> I love the logic of lower vs upper case letters in the selection of how
> an identifier should be used.  Unfortunately I can't support it.
> 
> Converting to anything other than init_user_ns will actually be an ABI
> break.  Which in practice should trump everything else.

I agree.  Initially I thought that my patch introduced only a minor
change to make the ABI more consistent, but if you think this change is
actually large enough to be considered a "not-wanted ABI break", I will
follow your decision.

> Further only the global root user can set this value, which largely
> implies that the program setting the core dump patter will expect the
> values to be in the initial user namespace.

Ok, I missed this.  The main source of my confusion is that the coredump
uses the current mount namespace to create files (I tested using a
Docker container with core_pattern set to something in /tmp) and the
global process/mount namespaces when using pipes (tested with
systemd-coredump).

> In practice your patch allows any user to put any uid they want on core
> files which seems to make the uid parameter useless.
> 
> So for all of the reasons above I think we need to print uids and gids
> in the initial user namespace unless explicitly requested to do so.

So be it.  I'll update my patches and send them again.  In order to make
things clearer, I also would like to modify the documentation of %u and
%g, with something like:

--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -197,8 +197,8 @@
 	%P	global pid (init PID namespace)
 	%i	tid
 	%I	global tid (init PID namespace)
-	%u	uid
-	%g	gid
+	%u	uid (in init user namespace)
+	%g	gid (in init user namespace)
 	%d	dump mode, matches PR_SET_DUMPABLE and
 		/proc/sys/fs/suid_dumpable
 	%s	signal number

Would such a change be accepted, and if yes is it better to put it in
its own commit or in the same as the one modifying the code?

Thanks,

Nicolas

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/2] coredump: use from_kuid/kgid when formatting corename
  2015-05-06 12:18           ` Nicolas Iooss
@ 2015-05-15  2:29             ` Nicolas Iooss
  2015-05-15  2:29               ` [PATCH v2 2/2] coredump: add __printf attribute to cn_*printf functions Nicolas Iooss
       [not found]               ` <1431656975-3563-1-git-send-email-nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
  0 siblings, 2 replies; 10+ messages in thread
From: Nicolas Iooss @ 2015-05-15  2:29 UTC (permalink / raw)
  To: Alexander Viro, linux-fsdevel, Andrew Morton, Eric W. Biederman
  Cc: linux-kernel, Linux Containers, Nicolas Iooss

When adding __printf attribute to cn_printf, gcc reports some issues:

  fs/coredump.c:213:5: warning: format '%d' expects argument of type
  'int', but argument 3 has type 'kuid_t' [-Wformat=]
       err = cn_printf(cn, "%d", cred->uid);
       ^
  fs/coredump.c:217:5: warning: format '%d' expects argument of type
  'int', but argument 3 has type 'kgid_t' [-Wformat=]
       err = cn_printf(cn, "%d", cred->gid);
       ^

These warnings come from the fact that the value of uid/gid needs to be
extracted from the kuid_t/kgid_t structure before being used as an
integer.  More precisely, cred->uid and cred->gid need to be converted
to either user-namespace uid/gid or to init_user_ns uid/gid.

Use init_user_ns in order not to break existing ABI, and document this
in Documentation/sysctl/kernel.txt.

While at it, format uid and gid values with %u instead of %d because
uid_t/__kernel_uid32_t and gid_t/__kernel_gid32_t are unsigned int.

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
---
 Documentation/sysctl/kernel.txt | 4 ++--
 fs/coredump.c                   | 8 ++++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index c831001c45f1..e1913f78f21e 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -197,8 +197,8 @@ core_pattern is used to specify a core dumpfile pattern name.
 	%P	global pid (init PID namespace)
 	%i	tid
 	%I	global tid (init PID namespace)
-	%u	uid
-	%g	gid
+	%u	uid (in initial user namespace)
+	%g	gid (in initial user namespace)
 	%d	dump mode, matches PR_SET_DUMPABLE and
 		/proc/sys/fs/suid_dumpable
 	%s	signal number
diff --git a/fs/coredump.c b/fs/coredump.c
index bbbe139ab280..833a57bc856c 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -209,11 +209,15 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
 				break;
 			/* uid */
 			case 'u':
-				err = cn_printf(cn, "%d", cred->uid);
+				err = cn_printf(cn, "%u",
+						from_kuid(&init_user_ns,
+							  cred->uid));
 				break;
 			/* gid */
 			case 'g':
-				err = cn_printf(cn, "%d", cred->gid);
+				err = cn_printf(cn, "%u",
+						from_kgid(&init_user_ns,
+							  cred->gid));
 				break;
 			case 'd':
 				err = cn_printf(cn, "%d",
-- 
2.4.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/2] coredump: add __printf attribute to cn_*printf functions
  2015-05-15  2:29             ` [PATCH v2 1/2] coredump: use from_kuid/kgid " Nicolas Iooss
@ 2015-05-15  2:29               ` Nicolas Iooss
  2015-05-15 14:54                 ` Eric W. Biederman
       [not found]               ` <1431656975-3563-1-git-send-email-nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
  1 sibling, 1 reply; 10+ messages in thread
From: Nicolas Iooss @ 2015-05-15  2:29 UTC (permalink / raw)
  To: Alexander Viro, linux-fsdevel, Andrew Morton, Eric W. Biederman
  Cc: linux-kernel, Linux Containers, Nicolas Iooss

This allows detecting improper format string at build time, like:

  fs/coredump.c:225:5: warning: format '%ld' expects argument of type
  'long int', but argument 3 has type 'int' [-Wformat=]
       err = cn_printf(cn, "%ld", cprm->siginfo->si_signo);
       ^

As si_signo is always an int, the format should be %d here.

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
---
 fs/coredump.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index 833a57bc856c..e52e0064feac 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -70,7 +70,8 @@ static int expand_corename(struct core_name *cn, int size)
 	return 0;
 }
 
-static int cn_vprintf(struct core_name *cn, const char *fmt, va_list arg)
+static __printf(2, 0) int cn_vprintf(struct core_name *cn, const char *fmt,
+				     va_list arg)
 {
 	int free, need;
 	va_list arg_copy;
@@ -93,7 +94,7 @@ again:
 	return -ENOMEM;
 }
 
-static int cn_printf(struct core_name *cn, const char *fmt, ...)
+static __printf(2, 3) int cn_printf(struct core_name *cn, const char *fmt, ...)
 {
 	va_list arg;
 	int ret;
@@ -105,7 +106,8 @@ static int cn_printf(struct core_name *cn, const char *fmt, ...)
 	return ret;
 }
 
-static int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
+static __printf(2, 3)
+int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
 {
 	int cur = cn->used;
 	va_list arg;
@@ -225,7 +227,8 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
 				break;
 			/* signal that caused the coredump */
 			case 's':
-				err = cn_printf(cn, "%ld", cprm->siginfo->si_signo);
+				err = cn_printf(cn, "%d",
+						cprm->siginfo->si_signo);
 				break;
 			/* UNIX time of coredump */
 			case 't': {
-- 
2.4.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] coredump: use from_kuid/kgid when formatting corename
       [not found]               ` <1431656975-3563-1-git-send-email-nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
@ 2015-05-15 14:52                 ` Eric W. Biederman
  0 siblings, 0 replies; 10+ messages in thread
From: Eric W. Biederman @ 2015-05-15 14:52 UTC (permalink / raw)
  To: Nicolas Iooss
  Cc: linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Linux Containers,
	Andrew Morton, Alexander Viro,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org> writes:

> When adding __printf attribute to cn_printf, gcc reports some issues:
>
>   fs/coredump.c:213:5: warning: format '%d' expects argument of type
>   'int', but argument 3 has type 'kuid_t' [-Wformat=]
>        err = cn_printf(cn, "%d", cred->uid);
>        ^
>   fs/coredump.c:217:5: warning: format '%d' expects argument of type
>   'int', but argument 3 has type 'kgid_t' [-Wformat=]
>        err = cn_printf(cn, "%d", cred->gid);
>        ^
>
> These warnings come from the fact that the value of uid/gid needs to be
> extracted from the kuid_t/kgid_t structure before being used as an
> integer.  More precisely, cred->uid and cred->gid need to be converted
> to either user-namespace uid/gid or to init_user_ns uid/gid.
>
> Use init_user_ns in order not to break existing ABI, and document this
> in Documentation/sysctl/kernel.txt.
>
> While at it, format uid and gid values with %u instead of %d because
> uid_t/__kernel_uid32_t and gid_t/__kernel_gid32_t are unsigned int.

Acked-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>

> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
> ---
>  Documentation/sysctl/kernel.txt | 4 ++--
>  fs/coredump.c                   | 8 ++++++--
>  2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
> index c831001c45f1..e1913f78f21e 100644
> --- a/Documentation/sysctl/kernel.txt
> +++ b/Documentation/sysctl/kernel.txt
> @@ -197,8 +197,8 @@ core_pattern is used to specify a core dumpfile pattern name.
>  	%P	global pid (init PID namespace)
>  	%i	tid
>  	%I	global tid (init PID namespace)
> -	%u	uid
> -	%g	gid
> +	%u	uid (in initial user namespace)
> +	%g	gid (in initial user namespace)
>  	%d	dump mode, matches PR_SET_DUMPABLE and
>  		/proc/sys/fs/suid_dumpable
>  	%s	signal number
> diff --git a/fs/coredump.c b/fs/coredump.c
> index bbbe139ab280..833a57bc856c 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -209,11 +209,15 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
>  				break;
>  			/* uid */
>  			case 'u':
> -				err = cn_printf(cn, "%d", cred->uid);
> +				err = cn_printf(cn, "%u",
> +						from_kuid(&init_user_ns,
> +							  cred->uid));
>  				break;
>  			/* gid */
>  			case 'g':
> -				err = cn_printf(cn, "%d", cred->gid);
> +				err = cn_printf(cn, "%u",
> +						from_kgid(&init_user_ns,
> +							  cred->gid));
>  				break;
>  			case 'd':
>  				err = cn_printf(cn, "%d",

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] coredump: add __printf attribute to cn_*printf functions
  2015-05-15  2:29               ` [PATCH v2 2/2] coredump: add __printf attribute to cn_*printf functions Nicolas Iooss
@ 2015-05-15 14:54                 ` Eric W. Biederman
  0 siblings, 0 replies; 10+ messages in thread
From: Eric W. Biederman @ 2015-05-15 14:54 UTC (permalink / raw)
  To: Nicolas Iooss
  Cc: Alexander Viro, linux-fsdevel, Andrew Morton, linux-kernel,
	Linux Containers

Nicolas Iooss <nicolas.iooss_linux@m4x.org> writes:

> This allows detecting improper format string at build time, like:
>
>   fs/coredump.c:225:5: warning: format '%ld' expects argument of type
>   'long int', but argument 3 has type 'int' [-Wformat=]
>        err = cn_printf(cn, "%ld", cprm->siginfo->si_signo);
>        ^
>
> As si_signo is always an int, the format should be %d here.

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
> ---
>  fs/coredump.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/fs/coredump.c b/fs/coredump.c
> index 833a57bc856c..e52e0064feac 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -70,7 +70,8 @@ static int expand_corename(struct core_name *cn, int size)
>  	return 0;
>  }
>  
> -static int cn_vprintf(struct core_name *cn, const char *fmt, va_list arg)
> +static __printf(2, 0) int cn_vprintf(struct core_name *cn, const char *fmt,
> +				     va_list arg)
>  {
>  	int free, need;
>  	va_list arg_copy;
> @@ -93,7 +94,7 @@ again:
>  	return -ENOMEM;
>  }
>  
> -static int cn_printf(struct core_name *cn, const char *fmt, ...)
> +static __printf(2, 3) int cn_printf(struct core_name *cn, const char *fmt, ...)
>  {
>  	va_list arg;
>  	int ret;
> @@ -105,7 +106,8 @@ static int cn_printf(struct core_name *cn, const char *fmt, ...)
>  	return ret;
>  }
>  
> -static int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
> +static __printf(2, 3)
> +int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
>  {
>  	int cur = cn->used;
>  	va_list arg;
> @@ -225,7 +227,8 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
>  				break;
>  			/* signal that caused the coredump */
>  			case 's':
> -				err = cn_printf(cn, "%ld", cprm->siginfo->si_signo);
> +				err = cn_printf(cn, "%d",
> +						cprm->siginfo->si_signo);
>  				break;
>  			/* UNIX time of coredump */
>  			case 't': {

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2015-05-15 14:58 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-16 10:28 [PATCH RESENT] coredump: fix cn_printf formatting warnings Nicolas Iooss
2015-04-16 10:50 ` Joe Perches
2015-05-03 10:34   ` [PATCH 1/2] coredump: use from_kuid/kgid_munged when formatting corename Nicolas Iooss
2015-05-03 10:34     ` [PATCH 2/2] coredump: add __printf attribute to cn_*printf functions Nicolas Iooss
     [not found]     ` <1430649246-32726-1-git-send-email-nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
2015-05-05 19:13       ` [PATCH 1/2] coredump: use from_kuid/kgid_munged when formatting corename Eric W. Biederman
     [not found]         ` <87bnhyhm7f.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
2015-05-06 12:18           ` Nicolas Iooss
2015-05-15  2:29             ` [PATCH v2 1/2] coredump: use from_kuid/kgid " Nicolas Iooss
2015-05-15  2:29               ` [PATCH v2 2/2] coredump: add __printf attribute to cn_*printf functions Nicolas Iooss
2015-05-15 14:54                 ` Eric W. Biederman
     [not found]               ` <1431656975-3563-1-git-send-email-nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
2015-05-15 14:52                 ` [PATCH v2 1/2] coredump: use from_kuid/kgid when formatting corename Eric W. Biederman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).