* [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
[parent not found: <1430649246-32726-1-git-send-email-nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>]
* 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
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).