Re: [PATCH] kmsg: Use vmalloc instead of kmalloc when writing

Re: [PATCH] kmsg: Use vmalloc instead of kmalloc when writing

[Greg KH]

On Fri, Mar 30, 2012 at 01:04:27PM -0400, Sasha Levin wrote:
> There are no size checks in kmsg_write(), and we try allocating enough
> memory to store everything userspace gave us, which may be too much for
> kmalloc to allocate.

Really?  Have you seen this fail?  As only root can do this, is this
really a problem?

> One option would be to limit it to something, but we can't come up with
> a number that would make sense.
> 
> Instead, just use vmalloc so that nothing would break with large amounts
> of data.

Are you sure this will work properly?  Have you tested it with large
amounts of data?

thanks,

greg k-h

Re: [PATCH] kmsg: Use vmalloc instead of kmalloc when writing

[Sasha Levin]

On Fri, Mar 30, 2012 at 6:30 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Mar 30, 2012 at 01:04:27PM -0400, Sasha Levin wrote:
>> There are no size checks in kmsg_write(), and we try allocating enough
>> memory to store everything userspace gave us, which may be too much for
>> kmalloc to allocate.
>
> Really?  Have you seen this fail?  As only root can do this, is this
> really a problem?

Only root, and a whole bunch of management software that dumps data
into /dev/kmsg (systemd and friends).

>> One option would be to limit it to something, but we can't come up with
>> a number that would make sense.
>>
>> Instead, just use vmalloc so that nothing would break with large amounts
>> of data.
>
> Are you sure this will work properly?  Have you tested it with large
> amounts of data?

My test was dumping 800mb of data into it, if there's anything else I
should try please let me know.

Thanks.

Re: [PATCH] kmsg: Use vmalloc instead of kmalloc when writing

[Greg KH]

On Fri, Mar 30, 2012 at 07:37:37PM +0300, Sasha Levin wrote:
> On Fri, Mar 30, 2012 at 6:30 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Fri, Mar 30, 2012 at 01:04:27PM -0400, Sasha Levin wrote:
> >> There are no size checks in kmsg_write(), and we try allocating enough
> >> memory to store everything userspace gave us, which may be too much for
> >> kmalloc to allocate.
> >
> > Really?  Have you seen this fail?  As only root can do this, is this
> > really a problem?
> 
> Only root, and a whole bunch of management software that dumps data
> into /dev/kmsg (systemd and friends).

Running as root, do any of these cause problems by asking for too much
memory here?  Is this something that needs to be addressed now, and in
stable kernels, or can it wait for 3.5?

> >> One option would be to limit it to something, but we can't come up with
> >> a number that would make sense.
> >>
> >> Instead, just use vmalloc so that nothing would break with large amounts
> >> of data.
> >
> > Are you sure this will work properly?  Have you tested it with large
> > amounts of data?
> 
> My test was dumping 800mb of data into it, if there's anything else I
> should try please let me know.

Did that fail before and now it works properly?

thanks,

greg k-h

[PATCH] kmsg: Use vmalloc instead of kmalloc when writing

[Sasha Levin]

There are no size checks in kmsg_write(), and we try allocating enough
memory to store everything userspace gave us, which may be too much for
kmalloc to allocate.

One option would be to limit it to something, but we can't come up with
a number that would make sense.

Instead, just use vmalloc so that nothing would break with large amounts
of data.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 drivers/char/mem.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index d6e9d08..e047783 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -815,7 +815,7 @@ static ssize_t kmsg_writev(struct kiocb *iocb, const struct iovec *iv,
 	ssize_t ret = -EFAULT;
 	size_t len = iov_length(iv, count);
 
-	line = kmalloc(len + 1, GFP_KERNEL);
+	line = vmalloc(len + 1);
 	if (line == NULL)
 		return -ENOMEM;
 
@@ -836,7 +836,7 @@ static ssize_t kmsg_writev(struct kiocb *iocb, const struct iovec *iv,
 	if (ret > len)
 		ret = len;
 out:
-	kfree(line);
+	vfree(line);
 	return ret;
 }
 
-- 
1.7.8.4

Re: [PATCH] kmsg: Use vmalloc instead of kmalloc when writing

[Sasha Levin]

On Fri, Mar 30, 2012 at 7:49 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Mar 30, 2012 at 07:37:37PM +0300, Sasha Levin wrote:
>> On Fri, Mar 30, 2012 at 6:30 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Fri, Mar 30, 2012 at 01:04:27PM -0400, Sasha Levin wrote:
>> >> There are no size checks in kmsg_write(), and we try allocating enough
>> >> memory to store everything userspace gave us, which may be too much for
>> >> kmalloc to allocate.
>> >
>> > Really?  Have you seen this fail?  As only root can do this, is this
>> > really a problem?
>>
>> Only root, and a whole bunch of management software that dumps data
>> into /dev/kmsg (systemd and friends).
>
> Running as root, do any of these cause problems by asking for too much
> memory here?  Is this something that needs to be addressed now, and in
> stable kernels, or can it wait for 3.5?

The only harm there is a kernel warning and the failure to write that
specific message to kmsg, combined with the fact that no one
complained about it before me I think it can probably wait for 3.5.

>> >> One option would be to limit it to something, but we can't come up with
>> >> a number that would make sense.
>> >>
>> >> Instead, just use vmalloc so that nothing would break with large amounts
>> >> of data.
>> >
>> > Are you sure this will work properly?  Have you tested it with large
>> > amounts of data?
>>
>> My test was dumping 800mb of data into it, if there's anything else I
>> should try please let me know.
>
> Did that fail before and now it works properly?

That's correct.

Re: [PATCH] kmsg: Use vmalloc instead of kmalloc when writing

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1475 bytes --]

On Fri, 30 Mar 2012, Greg KH wrote:
> On Fri, Mar 30, 2012 at 07:37:37PM +0300, Sasha Levin wrote:
> > On Fri, Mar 30, 2012 at 6:30 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > On Fri, Mar 30, 2012 at 01:04:27PM -0400, Sasha Levin wrote:
> > >> There are no size checks in kmsg_write(), and we try allocating enough
> > >> memory to store everything userspace gave us, which may be too much for
> > >> kmalloc to allocate.
> > >
> > > Really?  Have you seen this fail?  As only root can do this, is this
> > > really a problem?
> > 
> > Only root, and a whole bunch of management software that dumps data
> > into /dev/kmsg (systemd and friends).
> 
> Running as root, do any of these cause problems by asking for too much
> memory here? 

Running as root is not a guarantee for correctness. So the syscall
should cope with bogus requests from user space and not rely on the
sanity of anything. Looking at the main users which polute dmesg I'm
inclined to assume insanity in the first place.

As Sasha pointed out there is either the variant to use vmalloc and
grant any write size or limit the size to something sensible. Though
given the users of this, coming up with something sensible might be a
problem.

> Is this something that needs to be addressed now, and in
> stable kernels, or can it wait for 3.5?

Yes, it want's to be addressed now and it want's to be in stable as
well. syscalls which have no bound checking are evil, no matter what.
 
Thanks,

	tglx

Re: [PATCH] kmsg: Use vmalloc instead of kmalloc when writing

[Greg KH]

On Fri, Mar 30, 2012 at 10:35:46PM +0200, Thomas Gleixner wrote:
> On Fri, 30 Mar 2012, Greg KH wrote:
> > On Fri, Mar 30, 2012 at 07:37:37PM +0300, Sasha Levin wrote:
> > > On Fri, Mar 30, 2012 at 6:30 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > On Fri, Mar 30, 2012 at 01:04:27PM -0400, Sasha Levin wrote:
> > > >> There are no size checks in kmsg_write(), and we try allocating enough
> > > >> memory to store everything userspace gave us, which may be too much for
> > > >> kmalloc to allocate.
> > > >
> > > > Really?  Have you seen this fail?  As only root can do this, is this
> > > > really a problem?
> > > 
> > > Only root, and a whole bunch of management software that dumps data
> > > into /dev/kmsg (systemd and friends).
> > 
> > Running as root, do any of these cause problems by asking for too much
> > memory here? 
> 
> Running as root is not a guarantee for correctness. So the syscall
> should cope with bogus requests from user space and not rely on the
> sanity of anything. Looking at the main users which polute dmesg I'm
> inclined to assume insanity in the first place.
> 
> As Sasha pointed out there is either the variant to use vmalloc and
> grant any write size or limit the size to something sensible. Though
> given the users of this, coming up with something sensible might be a
> problem.
> 
> > Is this something that needs to be addressed now, and in
> > stable kernels, or can it wait for 3.5?
> 
> Yes, it want's to be addressed now and it want's to be in stable as
> well. syscalls which have no bound checking are evil, no matter what.

So, should we cap the size at something "super large" then as well?

thanks,

greg k-h

Re: [PATCH] kmsg: Use vmalloc instead of kmalloc when writing

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2044 bytes --]

On Fri, 30 Mar 2012, Greg KH wrote:

> On Fri, Mar 30, 2012 at 10:35:46PM +0200, Thomas Gleixner wrote:
> > On Fri, 30 Mar 2012, Greg KH wrote:
> > > On Fri, Mar 30, 2012 at 07:37:37PM +0300, Sasha Levin wrote:
> > > > On Fri, Mar 30, 2012 at 6:30 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > On Fri, Mar 30, 2012 at 01:04:27PM -0400, Sasha Levin wrote:
> > > > >> There are no size checks in kmsg_write(), and we try allocating enough
> > > > >> memory to store everything userspace gave us, which may be too much for
> > > > >> kmalloc to allocate.
> > > > >
> > > > > Really?  Have you seen this fail?  As only root can do this, is this
> > > > > really a problem?
> > > > 
> > > > Only root, and a whole bunch of management software that dumps data
> > > > into /dev/kmsg (systemd and friends).
> > > 
> > > Running as root, do any of these cause problems by asking for too much
> > > memory here? 
> > 
> > Running as root is not a guarantee for correctness. So the syscall
> > should cope with bogus requests from user space and not rely on the
> > sanity of anything. Looking at the main users which polute dmesg I'm
> > inclined to assume insanity in the first place.
> > 
> > As Sasha pointed out there is either the variant to use vmalloc and
> > grant any write size or limit the size to something sensible. Though
> > given the users of this, coming up with something sensible might be a
> > problem.
> > 
> > > Is this something that needs to be addressed now, and in
> > > stable kernels, or can it wait for 3.5?
> > 
> > Yes, it want's to be addressed now and it want's to be in stable as
> > well. syscalls which have no bound checking are evil, no matter what.
> 
> So, should we cap the size at something "super large" then as well?

I think so. This is an interface to inject stuff into dmesg. Limiting
that to a reasonable size makes sense. We can probably limit it to
something small like 1024, but I don't know about the "ideas" of those
folks who think that it's a great idea to do it at all.

Thanks,

	tglx

Re: [PATCH] kmsg: Use vmalloc instead of kmalloc when writing

[Arnd Bergmann]

On Friday 30 March 2012, Thomas Gleixner wrote:
> I think so. This is an interface to inject stuff into dmesg. Limiting
> that to a reasonable size makes sense. We can probably limit it to
> something small like 1024, but I don't know about the "ideas" of those
> folks who think that it's a great idea to do it at all.

I guess a page would be a reasonable size, similar to what we do for
sysfs. 

	Arnd

Re: [PATCH] kmsg: Use vmalloc instead of kmalloc when writing

[Greg KH]

On Fri, Mar 30, 2012 at 09:05:52PM +0000, Arnd Bergmann wrote:
> On Friday 30 March 2012, Thomas Gleixner wrote:
> > I think so. This is an interface to inject stuff into dmesg. Limiting
> > that to a reasonable size makes sense. We can probably limit it to
> > something small like 1024, but I don't know about the "ideas" of those
> > folks who think that it's a great idea to do it at all.
> 
> I guess a page would be a reasonable size, similar to what we do for
> sysfs. 

Ok. Sasha, as you seem to have noticed this, care to dig in syslog and
systemd to get an idea of the buffer sizes they are expecting to pass
into kmsg, and if they can handle a short write properly?  If so,
restricting it to a page is fine with me, otherwise we might want to
make it a bit bigger.

thanks,

greg k-h

Re: [PATCH] kmsg: Use vmalloc instead of kmalloc when writing

[Thomas Gleixner]

On Fri, 30 Mar 2012, Arnd Bergmann wrote:

> On Friday 30 March 2012, Thomas Gleixner wrote:
> > I think so. This is an interface to inject stuff into dmesg. Limiting
> > that to a reasonable size makes sense. We can probably limit it to
> > something small like 1024, but I don't know about the "ideas" of those
> > folks who think that it's a great idea to do it at all.
> 
> I guess a page would be a reasonable size, similar to what we do for
> sysfs. 

Fine with me.

Re: [PATCH] kmsg: Use vmalloc instead of kmalloc when writing

[Sasha Levin]

On Fri, Mar 30, 2012 at 11:17 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Mar 30, 2012 at 09:05:52PM +0000, Arnd Bergmann wrote:
>> On Friday 30 March 2012, Thomas Gleixner wrote:
>> > I think so. This is an interface to inject stuff into dmesg. Limiting
>> > that to a reasonable size makes sense. We can probably limit it to
>> > something small like 1024, but I don't know about the "ideas" of those
>> > folks who think that it's a great idea to do it at all.
>>
>> I guess a page would be a reasonable size, similar to what we do for
>> sysfs.
>
> Ok. Sasha, as you seem to have noticed this, care to dig in syslog and
> systemd to get an idea of the buffer sizes they are expecting to pass
> into kmsg, and if they can handle a short write properly?  If so,
> restricting it to a page is fine with me, otherwise we might want to
> make it a bit bigger.

systemd seems to use posix LINE_MAX sized buffers, syslog-ng uses
dynamic strings, but it chews them one line at the time.

Re: [PATCH] kmsg: Use vmalloc instead of kmalloc when writing

[Greg KH]

On Sat, Mar 31, 2012 at 12:02:39AM +0200, Sasha Levin wrote:
> On Fri, Mar 30, 2012 at 11:17 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Fri, Mar 30, 2012 at 09:05:52PM +0000, Arnd Bergmann wrote:
> >> On Friday 30 March 2012, Thomas Gleixner wrote:
> >> > I think so. This is an interface to inject stuff into dmesg. Limiting
> >> > that to a reasonable size makes sense. We can probably limit it to
> >> > something small like 1024, but I don't know about the "ideas" of those
> >> > folks who think that it's a great idea to do it at all.
> >>
> >> I guess a page would be a reasonable size, similar to what we do for
> >> sysfs.
> >
> > Ok. Sasha, as you seem to have noticed this, care to dig in syslog and
> > systemd to get an idea of the buffer sizes they are expecting to pass
> > into kmsg, and if they can handle a short write properly?  If so,
> > restricting it to a page is fine with me, otherwise we might want to
> > make it a bit bigger.
> 
> systemd seems to use posix LINE_MAX sized buffers, syslog-ng uses
> dynamic strings, but it chews them one line at the time.

Ok, care to update this patch with a max size?

And again, does systemd and syslog-ng handle short writes properly?

thanks,

greg k-h

Re: [PATCH] kmsg: Use vmalloc instead of kmalloc when writing

[Kay Sievers]

On Sat, Mar 31, 2012 at 01:43, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sat, Mar 31, 2012 at 12:02:39AM +0200, Sasha Levin wrote:
>> On Fri, Mar 30, 2012 at 11:17 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Fri, Mar 30, 2012 at 09:05:52PM +0000, Arnd Bergmann wrote:
>> >> On Friday 30 March 2012, Thomas Gleixner wrote:
>> >> > I think so. This is an interface to inject stuff into dmesg. Limiting
>> >> > that to a reasonable size makes sense. We can probably limit it to
>> >> > something small like 1024, but I don't know about the "ideas" of those
>> >> > folks who think that it's a great idea to do it at all.
>> >>
>> >> I guess a page would be a reasonable size, similar to what we do for
>> >> sysfs.
>> >
>> > Ok. Sasha, as you seem to have noticed this, care to dig in syslog and
>> > systemd to get an idea of the buffer sizes they are expecting to pass
>> > into kmsg, and if they can handle a short write properly?  If so,
>> > restricting it to a page is fine with me, otherwise we might want to
>> > make it a bit bigger.
>>
>> systemd seems to use posix LINE_MAX sized buffers, syslog-ng uses
>> dynamic strings, but it chews them one line at the time.
>
> Ok, care to update this patch with a max size?
>
> And again, does systemd and syslog-ng handle short writes properly?

Printk has a static scratch buffer of 1024, we can not really process
more than that, so we can limit the /dev/kmsg write() to the same
size, I guess.

Thanks,
Kay

Re: [PATCH] kmsg: Use vmalloc instead of kmalloc when writing

[Joe Perches]

On Fri, 2012-03-30 at 21:05 +0000, Arnd Bergmann wrote:
> On Friday 30 March 2012, Thomas Gleixner wrote:
> > I think so. This is an interface to inject stuff into dmesg. Limiting
> > that to a reasonable size makes sense. We can probably limit it to
> > something small like 1024, but I don't know about the "ideas" of those
> > folks who think that it's a great idea to do it at all.

Per line dmesg output is limited to 1024 anyway.

Re: [PATCH] kmsg: Use vmalloc instead of kmalloc when writing

[Sasha Levin]

On Sat, Mar 31, 2012 at 2:02 AM, Kay Sievers <kay@vrfy.org> wrote:
> On Sat, Mar 31, 2012 at 01:43, Greg KH <gregkh@linuxfoundation.org> wrote:
>> On Sat, Mar 31, 2012 at 12:02:39AM +0200, Sasha Levin wrote:
>>> On Fri, Mar 30, 2012 at 11:17 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>> > On Fri, Mar 30, 2012 at 09:05:52PM +0000, Arnd Bergmann wrote:
>>> >> On Friday 30 March 2012, Thomas Gleixner wrote:
>>> >> > I think so. This is an interface to inject stuff into dmesg. Limiting
>>> >> > that to a reasonable size makes sense. We can probably limit it to
>>> >> > something small like 1024, but I don't know about the "ideas" of those
>>> >> > folks who think that it's a great idea to do it at all.
>>> >>
>>> >> I guess a page would be a reasonable size, similar to what we do for
>>> >> sysfs.
>>> >
>>> > Ok. Sasha, as you seem to have noticed this, care to dig in syslog and
>>> > systemd to get an idea of the buffer sizes they are expecting to pass
>>> > into kmsg, and if they can handle a short write properly?  If so,
>>> > restricting it to a page is fine with me, otherwise we might want to
>>> > make it a bit bigger.
>>>
>>> systemd seems to use posix LINE_MAX sized buffers, syslog-ng uses
>>> dynamic strings, but it chews them one line at the time.
>>
>> Ok, care to update this patch with a max size?
>>
>> And again, does systemd and syslog-ng handle short writes properly?
>
> Printk has a static scratch buffer of 1024, we can not really process
> more than that, so we can limit the /dev/kmsg write() to the same
> size, I guess.

That's odd. I've tested it by writing 8000 chars into /dev/kmsg, and
all of them came out on the printk, and I saw all of them in my dmesg.

This means that while printk may be somehow limited to 1024, it's
still possible to dump more than that into dmesg.

Re: [PATCH] kmsg: Use vmalloc instead of kmalloc when writing

[Sasha Levin]

On Sat, Mar 31, 2012 at 1:43 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> Ok, care to update this patch with a max size?

Are there any objections to using PAGE_SIZE? If not, I'll send a revised patch.