[REGRESSION PATCH] vsprintf: increase sizeof precision in printf_spec

[REGRESSION PATCH] vsprintf: increase sizeof precision in printf_spec

[Eric Paris]

Patch ef0658f3de484bf9b173639cd47544584e01efa5 changed the precision field
from and int to an s8.  Problem is that we have code which uses a much larger
precision in the kernel.  An example would in the audit code where we have:

vsnprintf(...,..., " msg='%.1024s'", (char *)data);

which causes precision to be too large and end up truncating to nothing.
Raising the size of the precision fixes the audit system issue.  It also does
not affect the alignment of the struct according to pahole and is still
approprietely packed.

Signed-off-by: Eric Paris <eparis@redhat.com>
---

 lib/vsprintf.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 24112e5..a957d3f 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -412,7 +412,7 @@ struct printf_spec {
 	s16	field_width;	/* width of output field */
 	u8	flags;		/* flags to number() */
 	u8	base;
-	s8	precision;	/* # of digits/chars */
+	s16	precision;	/* # of digits/chars */
 	u8	qualifier;
 };
 

Re: [REGRESSION PATCH] vsprintf: increase sizeof precision in printf_spec

[Joe Perches]

On Tue, 2010-04-13 at 21:13 -0400, Eric Paris wrote:
> Patch ef0658f3de484bf9b173639cd47544584e01efa5 changed the precision field
> from and int to an s8.  Problem is that we have code which uses a much larger
> precision in the kernel.  An example would in the audit code where we have:
> 
> vsnprintf(...,..., " msg='%.1024s'", (char *)data);
> 
> which causes precision to be too large and end up truncating to nothing.
> Raising the size of the precision fixes the audit system issue.  It also does
> not affect the alignment of the struct according to pahole and is still
> approprietely packed.

I don't see how it could be appropriately packed.

This is the structure now:

struct printf_spec {
	u16	type;
	s16	field_width;	/* width of output field */
	u8	flags;		/* flags to number() */
	u8	base;
	s8	precision;	/* # of digits/chars */
	u8	qualifier;
};

Adding another char should make the structure larger than 64 bits.

type isn't currently required to be u16.
It could be u8.

Perhaps this is better.

struct printf_spec {
	u8 type;
	u8 flags;		/* flags to number() */
	u8 base;
	u8 qualifier;
	s16 field_width;	/* width of output field */
	s16 precision;		/* # of digits/chars */
} __attribute__((packed));

Re: [REGRESSION PATCH] vsprintf: increase sizeof precision in printf_spec

[Eric Paris]

On Tue, 2010-04-13 at 18:33 -0700, Joe Perches wrote:
> On Tue, 2010-04-13 at 21:13 -0400, Eric Paris wrote:
> > Patch ef0658f3de484bf9b173639cd47544584e01efa5 changed the precision field
> > from and int to an s8.  Problem is that we have code which uses a much larger
> > precision in the kernel.  An example would in the audit code where we have:
> > 
> > vsnprintf(...,..., " msg='%.1024s'", (char *)data);
> > 
> > which causes precision to be too large and end up truncating to nothing.
> > Raising the size of the precision fixes the audit system issue.  It also does
> > not affect the alignment of the struct according to pahole and is still
> > approprietely packed.
> 
> I don't see how it could be appropriately packed.

I was just saying there was no padding inside the struct, although you
are right about it now being longer than 64.

> type isn't currently required to be u16.
> It could be u8.
> 
> Perhaps this is better.
> 
> struct printf_spec {
> 	u8 type;
> 	u8 flags;		/* flags to number() */
> 	u8 base;
> 	u8 qualifier;
> 	s16 field_width;	/* width of output field */
> 	s16 precision;		/* # of digits/chars */
> } __attribute__((packed));

If we don't need the size in type I'm happy with this.

But what does __attribute__((packed)) buy us?  I always thought that was
frowned upon because it could put us in a position where if it does
affect the layout it destroys performance on systems where unaligned
access matters.  I don't think it is going to affect the alignment of
this struct, so I don't understand why we should add it.  But maybe I'm
misinformed....

I'll gladly resend with u8 type and s16 precision if that's the best
solution.

-Eric

Re: [REGRESSION PATCH] vsprintf: increase sizeof precision in printf_spec

[Joe Perches]

On Tue, 2010-04-13 at 22:44 -0400, Eric Paris wrote:
> On Tue, 2010-04-13 at 18:33 -0700, Joe Perches wrote:
> > On Tue, 2010-04-13 at 21:13 -0400, Eric Paris wrote:
> > > Patch ef0658f3de484bf9b173639cd47544584e01efa5 changed the precision field
> > > from and int to an s8.  Problem is that we have code which uses a much larger
> > > precision in the kernel.  An example would in the audit code where we have:
> > > 
> > > vsnprintf(...,..., " msg='%.1024s'", (char *)data);
> > > 
> > > which causes precision to be too large and end up truncating to nothing.
> > > Raising the size of the precision fixes the audit system issue.  It also does
> > > not affect the alignment of the struct according to pahole and is still
> > > approprietely packed.
> > 
> > I don't see how it could be appropriately packed.
> 
> I was just saying there was no padding inside the struct, although you
> are right about it now being longer than 64.

Which is bad.

> But what does __attribute__((packed)) buy us?

It could force the size to be 64 bits on more platforms.

> I'll gladly resend with u8 type and s16 precision if that's the best
> solution.

Reordering struct members to keep width and precision
together seems appropriate.  The attribute may not be.

struct printf_spec {
	u8 type;
	u8 flags;		/* flags to number() */
	u8 base;
	u8 qualifier;
	s16 field_width;	/* width of output field */
	s16 precision;		/* # of digits/chars */
};

Re: [REGRESSION PATCH] vsprintf: increase sizeof precision in printf_spec

[Justin P. mattock]

On 04/13/2010 08:01 PM, Joe Perches wrote:
> On Tue, 2010-04-13 at 22:44 -0400, Eric Paris wrote:
>> On Tue, 2010-04-13 at 18:33 -0700, Joe Perches wrote:
>>> On Tue, 2010-04-13 at 21:13 -0400, Eric Paris wrote:
>>>> Patch ef0658f3de484bf9b173639cd47544584e01efa5 changed the precision field
>>>> from and int to an s8.  Problem is that we have code which uses a much larger
>>>> precision in the kernel.  An example would in the audit code where we have:
>>>>
>>>> vsnprintf(...,..., " msg='%.1024s'", (char *)data);
>>>>
>>>> which causes precision to be too large and end up truncating to nothing.
>>>> Raising the size of the precision fixes the audit system issue.  It also does
>>>> not affect the alignment of the struct according to pahole and is still
>>>> approprietely packed.
>>>
>>> I don't see how it could be appropriately packed.
>>
>> I was just saying there was no padding inside the struct, although you
>> are right about it now being longer than 64.
>
> Which is bad.
>
>> But what does __attribute__((packed)) buy us?
>
> It could force the size to be 64 bits on more platforms.
>
>> I'll gladly resend with u8 type and s16 precision if that's the best
>> solution.
>
> Reordering struct members to keep width and precision
> together seems appropriate.  The attribute may not be.
>
> struct printf_spec {
> 	u8 type;
> 	u8 flags;		/* flags to number() */
> 	u8 base;
> 	u8 qualifier;
> 	s16 field_width;	/* width of output field */
> 	s16 precision;		/* # of digits/chars */
> };
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>


o.k. just added this patch from the first post,
and the avc's look complete.(I'll keep an eye on
nscd to make sure those avc's are complete as well).

looks good over here.

Justin P. Mattock

Re: [REGRESSION PATCH] vsprintf: increase sizeof precision in printf_spec

[Frederic Weisbecker]

On Tue, Apr 13, 2010 at 08:01:42PM -0700, Joe Perches wrote:
> On Tue, 2010-04-13 at 22:44 -0400, Eric Paris wrote:
> > On Tue, 2010-04-13 at 18:33 -0700, Joe Perches wrote:
> > > On Tue, 2010-04-13 at 21:13 -0400, Eric Paris wrote:
> > > > Patch ef0658f3de484bf9b173639cd47544584e01efa5 changed the precision field
> > > > from and int to an s8.  Problem is that we have code which uses a much larger
> > > > precision in the kernel.  An example would in the audit code where we have:
> > > > 
> > > > vsnprintf(...,..., " msg='%.1024s'", (char *)data);
> > > > 
> > > > which causes precision to be too large and end up truncating to nothing.
> > > > Raising the size of the precision fixes the audit system issue.  It also does
> > > > not affect the alignment of the struct according to pahole and is still
> > > > approprietely packed.
> > > 
> > > I don't see how it could be appropriately packed.
> > 
> > I was just saying there was no padding inside the struct, although you
> > are right about it now being longer than 64.
> 
> Which is bad.
> 
> > But what does __attribute__((packed)) buy us?
> 
> It could force the size to be 64 bits on more platforms.
> 
> > I'll gladly resend with u8 type and s16 precision if that's the best
> > solution.
> 
> Reordering struct members to keep width and precision
> together seems appropriate.  The attribute may not be.
> 
> struct printf_spec {
> 	u8 type;
> 	u8 flags;		/* flags to number() */
> 	u8 base;
> 	u8 qualifier;
> 	s16 field_width;	/* width of output field */
> 	s16 precision;		/* # of digits/chars */
> };


Yeah, we should avoid the attribute. Packing struct should be
done for pretty special cases, not to fix padding holes, because
the hole problem would be turned into an alignment access unefficiency.

[REGRESSION PATCH V2] vsprintf: Change struct printf_spec.precision from s8 to s16

[Joe Perches]

Commit ef0658f3de484bf9b173639cd47544584e01efa5 changed precision
from int to s8.

There is existing kernel code that uses a larger precision.

An example from the audit code:
	vsnprintf(...,..., " msg='%.1024s'", (char *)data);
which overflows precision and truncates to nothing.

Extending precision size fixes the audit system issue.

Other changes:

Change the size of the struct printf_spec.type from u16 to u8 so
sizeof(struct printf_spec) stays as small as possible.
Reorder the struct members so sizeof(struct printf_spec) remains 64 bits
without alignment holes.
Document the struct members a bit more.

Original-patch-by: Eric Paris <eparis@redhat.com>
Signed-off-by: Joe Perches <joe@perches.com>
---
 lib/vsprintf.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 24112e5..7376b7c 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -408,12 +408,12 @@ enum format_type {
 };
 
 struct printf_spec {
-	u16	type;
-	s16	field_width;	/* width of output field */
+	u8	type;		/* format_type enum */
 	u8	flags;		/* flags to number() */
-	u8	base;
-	s8	precision;	/* # of digits/chars */
-	u8	qualifier;
+	u8	base;		/* number base, 8, 10 or 16 only */
+	u8	qualifier;	/* number qualifier, one of 'hHlLtzZ' */
+	s16	field_width;	/* width of output field */
+	s16	precision;	/* # of digits/chars */
 };
 
 static char *number(char *buf, char *end, unsigned long long num,

Re: [REGRESSION PATCH V2] vsprintf: Change struct printf_spec.precision from s8 to s16

[Justin P. mattock]

On 04/14/2010 09:27 AM, Joe Perches wrote:
> Commit ef0658f3de484bf9b173639cd47544584e01efa5 changed precision
> from int to s8.
>
> There is existing kernel code that uses a larger precision.
>
> An example from the audit code:
> 	vsnprintf(...,..., " msg='%.1024s'", (char *)data);
> which overflows precision and truncates to nothing.
>
> Extending precision size fixes the audit system issue.
>
> Other changes:
>
> Change the size of the struct printf_spec.type from u16 to u8 so
> sizeof(struct printf_spec) stays as small as possible.
> Reorder the struct members so sizeof(struct printf_spec) remains 64 bits
> without alignment holes.
> Document the struct members a bit more.
>
> Original-patch-by: Eric Paris<eparis@redhat.com>
> Signed-off-by: Joe Perches<joe@perches.com>
> ---
>   lib/vsprintf.c |   10 +++++-----
>   1 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 24112e5..7376b7c 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -408,12 +408,12 @@ enum format_type {
>   };
>
>   struct printf_spec {
> -	u16	type;
> -	s16	field_width;	/* width of output field */
> +	u8	type;		/* format_type enum */
>   	u8	flags;		/* flags to number() */
> -	u8	base;
> -	s8	precision;	/* # of digits/chars */
> -	u8	qualifier;
> +	u8	base;		/* number base, 8, 10 or 16 only */
> +	u8	qualifier;	/* number qualifier, one of 'hHlLtzZ' */
> +	s16	field_width;	/* width of output field */
> +	s16	precision;	/* # of digits/chars */
>   };
>
>   static char *number(char *buf, char *end, unsigned long long num,
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

just applied:
the first patch in this thread is good,
as well as this one.

Tested-by: Justin P. Mattock <justinmattock@gmail.com>

Justin P. Mattock

Re: [REGRESSION PATCH V2] vsprintf: Change struct printf_spec.precision from s8 to s16

[Eric Paris]

On Wed, 2010-04-14 at 09:27 -0700, Joe Perches wrote:
> Commit ef0658f3de484bf9b173639cd47544584e01efa5 changed precision
> from int to s8.
> 
> There is existing kernel code that uses a larger precision.
> 
> An example from the audit code:
> 	vsnprintf(...,..., " msg='%.1024s'", (char *)data);
> which overflows precision and truncates to nothing.
> 
> Extending precision size fixes the audit system issue.
> 
> Other changes:
> 
> Change the size of the struct printf_spec.type from u16 to u8 so
> sizeof(struct printf_spec) stays as small as possible.
> Reorder the struct members so sizeof(struct printf_spec) remains 64 bits
> without alignment holes.
> Document the struct members a bit more.
> 
> Original-patch-by: Eric Paris <eparis@redhat.com>
> Signed-off-by: Joe Perches <joe@perches.com>

Sorry slow today

Acked-by: Eric Paris <eparis@redhat.com>
Tested-by:
Review-by:
anything else?