From: josh@joshtriplett.org
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
Fengguang Wu <fengguang.wu@intel.com>,
Iulia Manda <iulia.manda21@gmail.com>,
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
Fabian Frederick <fabf@skynet.be>,
Linux Memory Management List <linux-mm@kvack.org>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] devpts: If initialization failed, don't crash when opening /dev/ptmx
Date: Thu, 7 May 2015 17:37:48 -0700 [thread overview]
Message-ID: <20150508003748.GA1033@cloud> (raw)
In-Reply-To: <20150507155919.16ab7177e4956d8f47803750@linux-foundation.org>
On Thu, May 07, 2015 at 03:59:19PM -0700, Andrew Morton wrote:
> On Wed, 6 May 2015 17:35:47 -0700 Josh Triplett <josh@joshtriplett.org> wrote:
>
> > If devpts failed to initialize, it would store an ERR_PTR in the global
> > devpts_mnt. A subsequent open of /dev/ptmx would call devpts_new_index,
> > which would dereference devpts_mnt and crash.
> >
> > Avoid storing invalid values in devpts_mnt; leave it NULL instead.
> > Make both devpts_new_index and devpts_pty_new fail gracefully with
> > ENODEV in that case, which then becomes the return value to the
> > userspace open call on /dev/ptmx.
>
> It looks like the system is pretty crippled if init_devptr_fs() fails.
> Can the user actually get access to consoles and do useful things in
> this situation? Maybe it would be better to just give up and panic?
Mounting devpts doesn't work without it, but you don't *need* to do that
to run a viable system. A full-featured terminal might be unhappy.
init=/bin/sh works, and a console login doesn't strictly require
/dev/pts. A substantial initramfs or rescue system should work without
/dev/pts mounted.
I think this falls under Linus's comments elsewhere about BUG versus
WARN. The system can continue and will function to some degree.
panic() is more suitable for "if I even return from this function,
horrible things will start happening". With this patch, all the
functions provided by devpts gracefully fail if devpts did, so I don't
see a good reason to panic().
> > @@ -676,12 +689,15 @@ static int __init init_devpts_fs(void)
> > struct ctl_table_header *table;
> >
> > if (!err) {
> > + static struct vfsmount *mnt;
>
> static is weird. I assume this was a braino?
Copy/paste issue, yes. Fixed in v2.
- Josh Triplett
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
WARNING: multiple messages have this Message-ID (diff)
From: josh@joshtriplett.org
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
Fengguang Wu <fengguang.wu@intel.com>,
Iulia Manda <iulia.manda21@gmail.com>,
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
Fabian Frederick <fabf@skynet.be>,
Linux Memory Management List <linux-mm@kvack.org>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] devpts: If initialization failed, don't crash when opening /dev/ptmx
Date: Thu, 7 May 2015 17:37:48 -0700 [thread overview]
Message-ID: <20150508003748.GA1033@cloud> (raw)
In-Reply-To: <20150507155919.16ab7177e4956d8f47803750@linux-foundation.org>
On Thu, May 07, 2015 at 03:59:19PM -0700, Andrew Morton wrote:
> On Wed, 6 May 2015 17:35:47 -0700 Josh Triplett <josh@joshtriplett.org> wrote:
>
> > If devpts failed to initialize, it would store an ERR_PTR in the global
> > devpts_mnt. A subsequent open of /dev/ptmx would call devpts_new_index,
> > which would dereference devpts_mnt and crash.
> >
> > Avoid storing invalid values in devpts_mnt; leave it NULL instead.
> > Make both devpts_new_index and devpts_pty_new fail gracefully with
> > ENODEV in that case, which then becomes the return value to the
> > userspace open call on /dev/ptmx.
>
> It looks like the system is pretty crippled if init_devptr_fs() fails.
> Can the user actually get access to consoles and do useful things in
> this situation? Maybe it would be better to just give up and panic?
Mounting devpts doesn't work without it, but you don't *need* to do that
to run a viable system. A full-featured terminal might be unhappy.
init=/bin/sh works, and a console login doesn't strictly require
/dev/pts. A substantial initramfs or rescue system should work without
/dev/pts mounted.
I think this falls under Linus's comments elsewhere about BUG versus
WARN. The system can continue and will function to some degree.
panic() is more suitable for "if I even return from this function,
horrible things will start happening". With this patch, all the
functions provided by devpts gracefully fail if devpts did, so I don't
see a good reason to panic().
> > @@ -676,12 +689,15 @@ static int __init init_devpts_fs(void)
> > struct ctl_table_header *table;
> >
> > if (!err) {
> > + static struct vfsmount *mnt;
>
> static is weird. I assume this was a braino?
Copy/paste issue, yes. Fixed in v2.
- Josh Triplett
next prev parent reply other threads:[~2015-05-08 0:37 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-07 0:35 [PATCH] devpts: If initialization failed, don't crash when opening /dev/ptmx Josh Triplett
2015-05-07 0:35 ` Josh Triplett
2015-05-07 17:52 ` Peter Hurley
2015-05-07 17:52 ` Peter Hurley
2015-05-07 18:34 ` josh
2015-05-07 18:34 ` josh
2015-05-07 19:26 ` [PATCHv2] " Josh Triplett
2015-05-07 19:26 ` Josh Triplett
2015-05-07 22:59 ` [PATCH] " Andrew Morton
2015-05-07 22:59 ` Andrew Morton
2015-05-07 23:24 ` Peter Hurley
2015-05-07 23:24 ` Peter Hurley
2015-05-08 0:37 ` josh [this message]
2015-05-08 0:37 ` josh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150508003748.GA1033@cloud \
--to=josh@joshtriplett.org \
--cc=akpm@linux-foundation.org \
--cc=fabf@skynet.be \
--cc=fengguang.wu@intel.com \
--cc=iulia.manda21@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=paulmck@linux.vnet.ibm.com \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.