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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20140923055156.GC11740@mtj.dyndns.org>
Date:	Tue, 23 Sep 2014 01:51:56 -0400
From:	Tejun Heo <tj@...nel.org>
To:	NeilBrown <neilb@...e.de>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kernfs: use stack-buf for small writes.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/