[PATCH] kernfs: use stack-buf for small writes.

[PATCH] kernfs: use stack-buf for small writes.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2131 bytes --]


For a write <= 128 characters, don't use kmalloc.

mdmon, part of mdadm, will sometimes need to write
to a sysfs file in order to allow writes to the array
to continue.  This is important to support RAID metadata
types that the kernel doesn't know about.

It is important that this write doesn't block on
memory allocation.  The safest way to ensure that is to
use an on-stack buffer.

Writes are always small, typically less than 10 characters.

Note that reads from a sysfs file are already safe due to the use for
seqfile.  The first read will allocate a buffer (m->buf) which will
be used for all subsequent reads.

Signed-off-by: NeilBrown <neilb@suse.de>

---
Hi Tejun,
 I wonder if you would consider this patch.
 When mdmon needs to update metadata after a device failure in an array
 there are two 'kmalloc' sources that can trigger deadlock if memory is tight
 and needs to be written to the array (which cannot be allowed until mdmon
 updates the metadata).
 One is in O_DIRECT writes which I have patches for.  The other is when
 writing to the sysfs file to tell md that it is safe to continue.
 This simple patch removes the second.

Thanks,
NeilBrown


diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index 4429d6d9217f..75b58669ce55 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -269,6 +269,7 @@ static ssize_t kernfs_fop_write(struct file *file, const char __user *user_buf,
 	const struct kernfs_ops *ops;
 	size_t len;
 	char *buf;
+	char stackbuf[129];
 
 	if (of->atomic_write_len) {
 		len = count;
@@ -278,7 +279,10 @@ static ssize_t kernfs_fop_write(struct file *file, const char __user *user_buf,
 		len = min_t(size_t, count, PAGE_SIZE);
 	}
 
-	buf = kmalloc(len + 1, GFP_KERNEL);
+	if (len < sizeof(stackbuf))
+		buf = stackbuf;
+	else
+		buf = kmalloc(len + 1, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
@@ -311,7 +315,8 @@ static ssize_t kernfs_fop_write(struct file *file, const char __user *user_buf,
 	if (len > 0)
 		*ppos += len;
 out_free:
-	kfree(buf);
+	if (buf != stackbuf)
+		kfree(buf);
 	return len;
 }
 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] kernfs: use stack-buf for small writes.

[Tejun Heo]

Hello, Neil.

On Tue, Sep 23, 2014 at 02:06:33PM +1000, NeilBrown wrote:
>  When mdmon needs to update metadata after a device failure in an array
>  there are two 'kmalloc' sources that can trigger deadlock if memory is tight
>  and needs to be written to the array (which cannot be allowed until mdmon
>  updates the metadata).
>  One is in O_DIRECT writes which I have patches for.  The other is when
>  writing to the sysfs file to tell md that it is safe to continue.
>  This simple patch removes the second.

Ugh... :( If this can't be avoided at all, I'd much prefer it to be
something explicit - a flag marking the file as needing a persistent
write buffer which is allocated on open.  "Small" writes on stack
feels way to implicit to me.

Thanks.

-- 
tejun

Re: [PATCH] kernfs: use stack-buf for small writes.

[Tejun Heo]

On Tue, Sep 23, 2014 at 02:06:33PM +1000, NeilBrown wrote:
...
> Note that reads from a sysfs file are already safe due to the use for
> seqfile.  The first read will allocate a buffer (m->buf) which will
> be used for all subsequent reads.

Hmmm?  How is seqfile safe?  Where would the seq op write to?

Thanks.

-- 
tejun

Re: [PATCH] kernfs: use stack-buf for small writes.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1353 bytes --]

On Tue, 23 Sep 2014 00:18:17 -0400 Tejun Heo <tj@kernel.org> wrote:

> On Tue, Sep 23, 2014 at 02:06:33PM +1000, NeilBrown wrote:
> ...
> > Note that reads from a sysfs file are already safe due to the use for
> > seqfile.  The first read will allocate a buffer (m->buf) which will
> > be used for all subsequent reads.
> 
> Hmmm?  How is seqfile safe?  Where would the seq op write to?

seqfile is only safe for reads.  sysfs via kernfs uses seq_read(), so there
is only a single allocation on the first read.

It doesn't really related to fixing writes, except to point out that only
writes need to be "fixed".  Reads already work.

Separately:

> Ugh... :( If this can't be avoided at all, I'd much prefer it to be
> something explicit - a flag marking the file as needing a persistent
> write buffer which is allocated on open.  "Small" writes on stack
> feels way to implicit to me.

How about if we add seq_getbuf() and seq_putbuf() to seqfile
which takes a 'struct seq_file' and a size and returns the ->buf
after making sure it is big enough.
It also claims and releases the seqfile ->lock.

Then we would be using the same buffer for reads and write.

Does that sound suitable?  It uses existing infrastructure and avoids having
to identify in advance which attributes it is important for.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] kernfs: use stack-buf for small writes.

[Tejun Heo]

Hello, Neil.

On Tue, Sep 23, 2014 at 02:46:50PM +1000, NeilBrown wrote:
> seqfile is only safe for reads.  sysfs via kernfs uses seq_read(), so there
> is only a single allocation on the first read.
> 
> It doesn't really related to fixing writes, except to point out that only
> writes need to be "fixed".  Reads already work.

Oh, I meant the buffer seqfile read op writes to, so it depends on the
fact that the allocation is only on the first read?  That seems
extremely brittle to me, especially for an issue which tends to be
difficult to reproduce.

> Separately:
> 
> > Ugh... :( If this can't be avoided at all, I'd much prefer it to be
> > something explicit - a flag marking the file as needing a persistent
> > write buffer which is allocated on open.  "Small" writes on stack
> > feels way to implicit to me.
> 
> How about if we add seq_getbuf() and seq_putbuf() to seqfile
> which takes a 'struct seq_file' and a size and returns the ->buf
> after making sure it is big enough.
> It also claims and releases the seqfile ->lock.
> 
> Then we would be using the same buffer for reads and write.
> 
> Does that sound suitable?  It uses existing infrastructure and avoids having
> to identify in advance which attributes it is important for.

I'd much rather keep things direct and make it explicitly allocate r/w
buffer(s) on open and disallow seq_file operations on such files.

Thanks.

-- 
tejun

Re: [PATCH] kernfs: use stack-buf for small writes.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2324 bytes --]

On Tue, 23 Sep 2014 00:55:49 -0400 Tejun Heo <tj@kernel.org> wrote:

> Hello, Neil.
> 
> On Tue, Sep 23, 2014 at 02:46:50PM +1000, NeilBrown wrote:
> > seqfile is only safe for reads.  sysfs via kernfs uses seq_read(), so there
> > is only a single allocation on the first read.
> > 
> > It doesn't really related to fixing writes, except to point out that only
> > writes need to be "fixed".  Reads already work.
> 
> Oh, I meant the buffer seqfile read op writes to, so it depends on the
> fact that the allocation is only on the first read?  That seems
> extremely brittle to me, especially for an issue which tends to be
> difficult to reproduce.

It is easy for user-space to ensure they read once before any critical time..

> 
> > Separately:
> > 
> > > Ugh... :( If this can't be avoided at all, I'd much prefer it to be
> > > something explicit - a flag marking the file as needing a persistent
> > > write buffer which is allocated on open.  "Small" writes on stack
> > > feels way to implicit to me.
> > 
> > How about if we add seq_getbuf() and seq_putbuf() to seqfile
> > which takes a 'struct seq_file' and a size and returns the ->buf
> > after making sure it is big enough.
> > It also claims and releases the seqfile ->lock.
> > 
> > Then we would be using the same buffer for reads and write.
> > 
> > Does that sound suitable?  It uses existing infrastructure and avoids having
> > to identify in advance which attributes it is important for.
> 
> I'd much rather keep things direct and make it explicitly allocate r/w
> buffer(s) on open and disallow seq_file operations on such files.

As far as I can tell, seq_read is used on all sysfs files that are
readable except for 'binary' files.  Are you suggesting all files that might
need to be accessed without a kmalloc have to be binary files?

Having to identify those files which are important in advance seems the more
"brittle" approach to me.  I would much rather it "just worked"

Would you prefer a new per-attribute flag which directed sysfs to
pre-allocate a full page, or a 'max_size' attribute which caused a buffer of
that size to be allocated on open?
The same size would be used to pre-allocate the seqfile buf (like
single_open_size does) if reads were supported.

Thanks,
NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] kernfs: use stack-buf for small writes.

[Tejun Heo]

On Tue, Sep 23, 2014 at 03:40:58PM +1000, NeilBrown wrote:
> > Oh, I meant the buffer seqfile read op writes to, so it depends on the
> > fact that the allocation is only on the first read?  That seems
> > extremely brittle to me, especially for an issue which tends to be
> > difficult to reproduce.
> 
> It is easy for user-space to ensure they read once before any critical time..

Sure, but it's a hard and subtle dependency on an extremely obscure
implementation detail.

> > I'd much rather keep things direct and make it explicitly allocate r/w
> > buffer(s) on open and disallow seq_file operations on such files.
> 
> As far as I can tell, seq_read is used on all sysfs files that are
> readable except for 'binary' files.  Are you suggesting all files that might
> need to be accessed without a kmalloc have to be binary files?

kernfs ->direct_read() callback doesn't go through seq_file.  sysfs
can be extended to support that for regular files, I guess.  Or just
make those special files binary?

> Having to identify those files which are important in advance seems the more
> "brittle" approach to me.  I would much rather it "just worked"

I disagree.  The files which shouldn't involve memory allocations must
be identified no matter what.  They're *very* special.  And the rules
that userland has to follow seem completely broken to me.  "Small"
writes are okay, whatever that means, and "small" reads are okay too
as long as it isn't the first read.  Ooh, BTW, if the second read ends
up expanding the initial buffer, it isn't okay - the initial boundary
is PAGE_SIZE and the buffer is expanded twice on each overflow.  How
are these rules okay?  This is borderline crazy.  In addition, the
read path involves a lot more code this way.  It ends up locking down
buffer policies of the whole seqfile implementation.

> Would you prefer a new per-attribute flag which directed sysfs to
> pre-allocate a full page, or a 'max_size' attribute which caused a buffer of
> that size to be allocated on open?
> The same size would be used to pre-allocate the seqfile buf (like
> single_open_size does) if reads were supported.

Yes but I really think we should avoid seqfile dependency.

Thanks.

-- 
tejun

Re: [PATCH] kernfs: use stack-buf for small writes.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2504 bytes --]

On Tue, 23 Sep 2014 01:51:56 -0400 Tejun Heo <tj@kernel.org> wrote:

> On Tue, Sep 23, 2014 at 03:40:58PM +1000, NeilBrown wrote:
> > > Oh, I meant the buffer seqfile read op writes to, so it depends on the
> > > fact that the allocation is only on the first read?  That seems
> > > extremely brittle to me, especially for an issue which tends to be
> > > difficult to reproduce.
> > 
> > It is easy for user-space to ensure they read once before any critical time..
> 
> Sure, but it's a hard and subtle dependency on an extremely obscure
> implementation detail.
> 
> > > I'd much rather keep things direct and make it explicitly allocate r/w
> > > buffer(s) on open and disallow seq_file operations on such files.
> > 
> > As far as I can tell, seq_read is used on all sysfs files that are
> > readable except for 'binary' files.  Are you suggesting all files that might
> > need to be accessed without a kmalloc have to be binary files?
> 
> kernfs ->direct_read() callback doesn't go through seq_file.  sysfs
> can be extended to support that for regular files, I guess.  Or just
> make those special files binary?
> 
> > Having to identify those files which are important in advance seems the more
> > "brittle" approach to me.  I would much rather it "just worked"
> 
> I disagree.  The files which shouldn't involve memory allocations must
> be identified no matter what.  They're *very* special.  And the rules
> that userland has to follow seem completely broken to me.  "Small"
> writes are okay, whatever that means, and "small" reads are okay too
> as long as it isn't the first read.  Ooh, BTW, if the second read ends
> up expanding the initial buffer, it isn't okay - the initial boundary
> is PAGE_SIZE and the buffer is expanded twice on each overflow.  How
> are these rules okay?  This is borderline crazy.  In addition, the
> read path involves a lot more code this way.  It ends up locking down
> buffer policies of the whole seqfile implementation.
> 
> > Would you prefer a new per-attribute flag which directed sysfs to
> > pre-allocate a full page, or a 'max_size' attribute which caused a buffer of
> > that size to be allocated on open?
> > The same size would be used to pre-allocate the seqfile buf (like
> > single_open_size does) if reads were supported.
> 
> Yes but I really think we should avoid seqfile dependency.

I'll see what I can do.

You didn't say if you preferred a flag or a 'max_size'.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] kernfs: use stack-buf for small writes.

[Tejun Heo]

Hello, Neil.

On Tue, Sep 23, 2014 at 04:11:44PM +1000, NeilBrown wrote:
> You didn't say if you preferred a flag or a 'max_size'.

Sorry, missed that.  No strong preference but a flag should be easier
and enough for now, right?

Thanks.

-- 
tejun

Re: [PATCH] kernfs: use stack-buf for small writes.

[Al Viro]

On Tue, Sep 23, 2014 at 02:06:33PM +1000, NeilBrown wrote:
> 
> For a write <= 128 characters, don't use kmalloc.
> 
> mdmon, part of mdadm, will sometimes need to write
> to a sysfs file in order to allow writes to the array
> to continue.  This is important to support RAID metadata
> types that the kernel doesn't know about.
> 
> It is important that this write doesn't block on
> memory allocation.  The safest way to ensure that is to
> use an on-stack buffer.

Is it, now?  What happens if caller of write(2) loses CPU and gets
the page it's writing from swapped out, just as it's getting to
issuing the actual syscall?  copy_from_user() will fault and cause
memory allocation for you...

> Writes are always small, typically less than 10 characters.

Translation: "but my userland code uses it _that_ way; surely, nobody
would do anything else..."

Re: [PATCH] kernfs: use stack-buf for small writes.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 5722 bytes --]

On Wed, 24 Sep 2014 20:17:27 +0100 Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Tue, Sep 23, 2014 at 02:06:33PM +1000, NeilBrown wrote:
> > 
> > For a write <= 128 characters, don't use kmalloc.
> > 
> > mdmon, part of mdadm, will sometimes need to write
> > to a sysfs file in order to allow writes to the array
> > to continue.  This is important to support RAID metadata
> > types that the kernel doesn't know about.
> > 
> > It is important that this write doesn't block on
> > memory allocation.  The safest way to ensure that is to
> > use an on-stack buffer.
> 
> Is it, now?  What happens if caller of write(2) loses CPU and gets
> the page it's writing from swapped out, just as it's getting to
> issuing the actual syscall?  copy_from_user() will fault and cause
> memory allocation for you...

That problem is easily solved with

     mlockall(MCL_CURRENT|MCL_FUTURE);

That will prevent any swapout.

I see this effort a bit like an extension to MCL_FUTURE.

A process that wants to avoid latency from the memory system can use
mlockall() and then pre-allocate any memory that it might need.
Then it can be sure it will not block on memory accesses.

However it is not possible to pre-allocate memory that will be used
by the kernel, such as in a write to a sysfs file.

So my real goal is to ensure that when MCL_FUTURE has been set, it is
possible to pre-allocate any kernel memory that might be needed.
For this case it was easiest to simply "not need" any kernel memory.

I have a patch which ensures that O_DIRECT writes to a block device will not
block on memory allocations.  It keeps the 'struct dio' around precisely if
MCL_FUTURE was used.  So after the first access (read or write) future writes
will not block in memory allocation.

An alternate approach might be to allow PF_MEMALLOC to be permanently set
from user-space, but I suspect that is too heavy handed and wouldn't be a
good solution.


A key point here is that it is the "process" rather than the "file" that is
special.   It isn't that accesses to the file should never block.  It is that
the process should be able to avoid blocking.  That is why I don't really
want to special-case a few sysfs attribute files.

> 
> > Writes are always small, typically less than 10 characters.
> 
> Translation: "but my userland code uses it _that_ way; surely, nobody
> would do anything else..."

That isn't how I see it.
These are sysfs attribute files.  By definition they contain precisely one
value - typically a bool, enum, or integer.
I realise that this "definition" is often stretched and sometimes completely
ignored, but it is largely adhered to and when it is violated it is more
often the "read" side than the "write" side.

So it isn't just "my userland code" - it is any sane code on any sane sysfs
attribute.  "typically less than 10" is probably not helpful, but the code
was for "less than 128 is all interesting cases" and that covers all numbers,
enums, bools and anything that isn't a gross violation of "one value".

But if you don't like a limit (and I do agree that it is usually good to
avoid arbitrary limits even though we already have an arbitrary limit of
PAGE_SIZE) we could do it like this.
This allows the userland code to pre-allocate a buffer  by performing a write.
I haven't made it conditional on
      current->mm->def_flags & VM_LOCKED
so it would only affect mlock(MCL_FUTURE) processes, but that would be easy.

Thanks for your comments,
NeilBrown


diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index 4429d6d9217f..a4d28f78cae1 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -278,40 +278,49 @@ static ssize_t kernfs_fop_write(struct file *file, const char __user *user_buf,
 		len = min_t(size_t, count, PAGE_SIZE);
 	}
 
-	buf = kmalloc(len + 1, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
-	if (copy_from_user(buf, user_buf, len)) {
-		len = -EFAULT;
-		goto out_free;
-	}
-	buf[len] = '\0';	/* guarantee string termination */
-
 	/*
 	 * @of->mutex nests outside active ref and is just to ensure that
 	 * the ops aren't called concurrently for the same open file.
+	 * Also protects of->buf.
 	 */
 	mutex_lock(&of->mutex);
 	if (!kernfs_get_active(of->kn)) {
 		mutex_unlock(&of->mutex);
-		len = -ENODEV;
-		goto out_free;
+		return -ENODEV;
 	}
 
+	if (of->buf && of->buflen >= len+1)
+		buf = of->buf;
+	else
+		buf = kmalloc(len + 1, GFP_KERNEL);
+	if (!buf) {
+		len = -ENOMEM;
+		goto out_unlock;
+	}
+	if (buf != of->buf) {
+		kfree(of->buf);
+		of->buf = buf;
+		of->buflen = len + 1;
+	}
+
+	if (copy_from_user(buf, user_buf, len)) {
+		len = -EFAULT;
+		goto out_unlock;
+	}
+	buf[len] = '\0';	/* guarantee string termination */
+
 	ops = kernfs_ops(of->kn);
 	if (ops->write)
 		len = ops->write(of, buf, len, *ppos);
 	else
 		len = -EINVAL;
 
-	kernfs_put_active(of->kn);
-	mutex_unlock(&of->mutex);
-
 	if (len > 0)
 		*ppos += len;
-out_free:
-	kfree(buf);
+
+out_unlock:
+	kernfs_put_active(of->kn);
+	mutex_unlock(&of->mutex);
 	return len;
 }
 
@@ -728,6 +737,7 @@ static int kernfs_fop_release(struct inode *inode, struct file *filp)
 
 	kernfs_put_open_node(kn, of);
 	seq_release(inode, filp);
+	kfree(of->buf);
 	kfree(of);
 
 	return 0;
diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
index 30faf797c2c3..c6addf635990 100644
--- a/include/linux/kernfs.h
+++ b/include/linux/kernfs.h
@@ -179,6 +179,8 @@ struct kernfs_open_file {
 	struct mutex		mutex;
 	int			event;
 	struct list_head	list;
+	char			*buf;
+	int			buflen;
 
 	size_t			atomic_write_len;
 	bool			mmapped;

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]