linux-kernel - Re: [PATCH 01/16] devpts: Attempting to get it right

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: <87twixgsnq.fsf@x220.int.ebiederm.org>
Date:	Tue, 19 Apr 2016 17:06:33 -0500
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	"H. Peter Anvin" <hpa@...or.com>,
	Andy Lutomirski <luto@...capital.net>, security@...ian.org,
	"security\@kernel.org" <security@...nel.org>,
	Al Viro <viro@...iv.linux.org.uk>,
	"security\@ubuntu.com \>\> security" <security@...ntu.com>,
	Peter Hurley <peter@...leysoftware.com>,
	Serge Hallyn <serge.hallyn@...ntu.com>,
	Willy Tarreau <w@....eu>,
	Aurelien Jarno <aurelien@...el32.net>,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	Jann Horn <jann@...jh.net>, Greg KH <greg@...ah.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Jiri Slaby <jslaby@...e.com>, Florian Weimer <fw@...eb.enyo.de>
Subject: Re: [PATCH 01/16] devpts: Attempting to get it right

Linus Torvalds <torvalds@...ux-foundation.org> writes:

> So I've looked at the devpts patches some more, and I'm still not happy 
> with them.
>
> And one thing I really detest about them is that the 16-patch series 
> didn't really make me warm and fuzzy in general. Some of the patches were 
> fine, but on the whole it all still looked rather hacky.
>
> So I started looking at the code with the intent of trying to clean things 
> up _gradually_, knowing roughly where we want to end up, but also trying 
> to make single patches that look sane on their own, and can be judged on 
> their own without any other patches or even any semantic arguments.
>
> And this appended patch is I think the first step.
>
> What this does is get rid of the horrible notion of having that
>
>     struct inode *ptmx_inode
>
> be the interface between the pty code and devpts. By de-emphasizing the 
> ptmx inode, a lot of things actually get cleaner, and we will have a much 
> saner way forward.
>
> The patch itself is actually fairly straightforward, and apart from some 
> locking cleanups it's pretty mechanical:
>
>  - the interfaces that devpts exposes all take "struct pts_fs_info *" 
>    instead of "struct inode *ptmx_inode" now.
>
>    NOTE! The "struct pts_fs_info" thing is a completely opaque structure 
>    as far as the pty driver is concerned: it's still declared entirely 
>    internally to devpts. So the pty code can't actually access it in any 
>    way, just pass it as a "cookie" to the devpts code.
>
>  - the "look up the pts fs info" is now a single clear operation, that 
>    also does the reference count increment on the pts superblock.
>
>    So "devpts_add/del_ref()" is gone, and replaced by a "lookup and get 
>    ref" operation (devpts_get_ref(inode)), along with a "put ref" op
>    (devpts_put_ref()).
>
>  - the pty master "tty->driver_data" field now contains the pts_fs_info, 
>    not the ptmx inode.
>
>  - because we don't care about the ptmx inode any more as some kind of 
>    base index, the ref counting can now drop the inode games - it just 
>    gets the ref on the superblock.
>
>  - the pts_fs_info now has a back-pointer to the super_block. That's so 
>    that we can easily look up the information we actually need. Although 
>    quite often, the pts fs info was actually all we wanted, and not having 
>    to look it up based on some magical inode makes things more
>    straightforward.
>
> Now, I haven't actually *tested* the patch very much: it compiles, it 
> boots, and my (fairly limited) environment still works, but that's by no 
> means exhaustive. So I may have screwed something up, but most of this was 
> really fairly straightforward.
>
> But more importantly, I think it all makes sense independently of anything 
> else. In particular, now that "devpts_get_ref(inode)" operation should 
> really be the *only* place we need to look up what devpts instance we're 
> associated with, and we do it exactly once, at ptmx_open() time.
>
> So I think this is a good step forward, while avoiding anything that could 
> be at all controversial.
>
> Comments about the patch?

Mostly I think it is six of one half dozen of the other, but given
driver_data is used so few times and that those times are very clear
whether we are accessing a master or a slave worth doing just for
clarity.

I have work inspired by this rolled into my code.  I will post shortly
after a little more testing.

Eric