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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 23 Sep 2014 15:40:58 +1000
From:	NeilBrown <neilb@...e.de>
To:	Tejun Heo <tj@...nel.org>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kernfs: use stack-buf for small writes.

On Tue, 23 Sep 2014 00:55:49 -0400 Tejun Heo <tj@...nel.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


Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)