Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1291810 > unrolled thread

[PATCH] pty: fix use after free of tty->driver_data

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH] pty: fix use after free of tty->driver_data "Herton R. Krzesinski" <herton@redhat.com> - 2015-12-15 04:30 +0100
    Re: [PATCH] pty: fix use after free of tty->driver_data Peter Hurley <peter@hurleysoftware.com> - 2015-12-15 18:40 +0100
      Re: [PATCH] pty: fix use after free of tty->driver_data "Herton R. Krzesinski" <herton@redhat.com> - 2015-12-15 19:10 +0100
        Re: [PATCH] pty: fix use after free of tty->driver_data "Herton R. Krzesinski" <herton@redhat.com> - 2015-12-15 20:30 +0100
          Re: [PATCH] pty: fix use after free of tty->driver_data Peter Hurley <peter@hurleysoftware.com> - 2015-12-15 21:00 +0100
            Re: [PATCH] pty: fix use after free of tty->driver_data "Herton R. Krzesinski" <herton@redhat.com> - 2015-12-15 21:40 +0100
              Re: [PATCH] pty: fix use after free of tty->driver_data Peter Hurley <peter@hurleysoftware.com> - 2015-12-15 21:40 +0100
        Re: [PATCH] pty: fix use after free of tty->driver_data "Herton R. Krzesinski" <herton@redhat.com> - 2015-12-29 19:00 +0100

#1291810 — [PATCH] pty: fix use after free of tty->driver_data

pty_unix98_shutdown allows a potential use after free of inode from
slave tty->driver_data: if final pty close is called with slave
tty_struct, and inode was released already by devpts_pty_kill at
pty_close, pty_unix98_shutdown will access stale data. If the evicted
inode is quickly reused again as another inode instance, this can
potentially break in the case the inode is on a different devpts
instance than the default devpts mount, not to mention a possible ops
if the inode_cache slab is destroyed and reused as something else.

Also there is an evident problem in case the last close is from a
opened "/dev/tty" which points to the master/slave pty: since in this
case any of the tty->driver_data can be stale, due to all references/
files being closed before (files related to ptmx/pts inodes set at
tty->driver_data), we have the possibility of referencing an already
freed inode.

The fix here is to keep a reference on the opened master ptmx inode.
We maintain the inode referenced until the final pty_unix98_shutdown,
and only pass this inode to devpts_kill_index.

Signed-off-by: Herton R. Krzesinski <herton@redhat.com>
Cc: <stable@vger.kernel.org>
---
 drivers/tty/pty.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index a45660f..90743b0 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -681,7 +681,14 @@ static void pty_unix98_remove(struct tty_driver *driver, struct tty_struct *tty)
 /* this is called once with whichever end is closed last */
 static void pty_unix98_shutdown(struct tty_struct *tty)
 {
-	devpts_kill_index(tty->driver_data, tty->index);
+	struct inode *ptmx_inode;
+
+	if (tty->driver->subtype == PTY_TYPE_MASTER)
+		ptmx_inode = tty->driver_data;
+	else
+		ptmx_inode = tty->link->driver_data;
+	devpts_kill_index(ptmx_inode, tty->index);
+	iput(ptmx_inode); /* drop reference we acquired at ptmx_open */
 }
 
 static const struct tty_operations ptm_unix98_ops = {
@@ -773,6 +780,15 @@ static int ptmx_open(struct inode *inode, struct file *filp)
 	set_bit(TTY_PTY_LOCK, &tty->flags); /* LOCK THE SLAVE */
 	tty->driver_data = inode;
 
+	/*
+	 * In the rare case all references to ptmx inode are dropped (files
+	 * closed), and we still have a device opened pointing to the
+	 * master/slave pair (eg., "/dev/tty" opened), we must make sure that
+	 * the inode is still valid when we call the final pty_unix98_shutdown:
+	 * thus we must hold an additional reference to the ptmx inode here
+	 */
+	ihold(inode);
+
 	tty_add_file(tty, filp);
 
 	slave_inode = devpts_pty_new(inode,
-- 
2.4.3

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

[toc] | [next] | [standalone]

#1292395

Hi Herton,

On 12/14/2015 07:29 PM, Herton R. Krzesinski wrote:
> pty_unix98_shutdown allows a potential use after free of inode from
> slave tty->driver_data: if final pty close is called with slave
> tty_struct, and inode was released already by devpts_pty_kill at
> pty_close, pty_unix98_shutdown will access stale data.

I'm not following this logic.

Suppose there is no open current tty alias for the slave pty; then
there is one inode reference for the master and N + 1 inode references
for however many times the slave has been opened.

If the master is closed first, the slave dentry is dropped so there
are now N slave inode references. When the last slave closes, the
slave inode is still valid when devpts_kill_index() is called.

So afaict, this problem only applies when /dev/tty is the final
close, and at no other time. [Not to minimize the scale of the problem:
quite often, /dev/tty would be the last file closed.]


> If the evicted inode is quickly reused again as another inode
> instance, this can potentially break in the case the inode is on a
> different devpts instance than the default devpts mount, not to
> mention a possible ops if the inode_cache slab is destroyed and
> reused as something else.
> 
> Also there is an evident problem in case the last close is from a
> opened "/dev/tty" which points to the master/slave pty:

/dev/tty can only refer to the slave pty.


> since in this
> case any of the tty->driver_data can be stale, due to all references/
> files being closed before (files related to ptmx/pts inodes set at
> tty->driver_data), we have the possibility of referencing an already
> freed inode.

As I wrote above, I believe this is the only possible circumstance
for which the file that is releasing could have stale pts inodes.


> The fix here is to keep a reference on the opened master ptmx inode.
> We maintain the inode referenced until the final pty_unix98_shutdown,
> and only pass this inode to devpts_kill_index.

Let me think some on your proposed solution.


> Signed-off-by: Herton R. Krzesinski <herton@redhat.com>
> Cc: <stable@vger.kernel.org>

Afaict, the stable tag goes back to the original implementation.
Did you research how far back the /dev/tty alias problem goes?

Regards,
Peter Hurley

> ---
>  drivers/tty/pty.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
> index a45660f..90743b0 100644
> --- a/drivers/tty/pty.c
> +++ b/drivers/tty/pty.c
> @@ -681,7 +681,14 @@ static void pty_unix98_remove(struct tty_driver *driver, struct tty_struct *tty)
>  /* this is called once with whichever end is closed last */
>  static void pty_unix98_shutdown(struct tty_struct *tty)
>  {
> -	devpts_kill_index(tty->driver_data, tty->index);
> +	struct inode *ptmx_inode;
> +
> +	if (tty->driver->subtype == PTY_TYPE_MASTER)
> +		ptmx_inode = tty->driver_data;
> +	else
> +		ptmx_inode = tty->link->driver_data;
> +	devpts_kill_index(ptmx_inode, tty->index);
> +	iput(ptmx_inode); /* drop reference we acquired at ptmx_open */
>  }
>  
>  static const struct tty_operations ptm_unix98_ops = {
> @@ -773,6 +780,15 @@ static int ptmx_open(struct inode *inode, struct file *filp)
>  	set_bit(TTY_PTY_LOCK, &tty->flags); /* LOCK THE SLAVE */
>  	tty->driver_data = inode;
>  
> +	/*
> +	 * In the rare case all references to ptmx inode are dropped (files
> +	 * closed), and we still have a device opened pointing to the
> +	 * master/slave pair (eg., "/dev/tty" opened), we must make sure that
> +	 * the inode is still valid when we call the final pty_unix98_shutdown:
> +	 * thus we must hold an additional reference to the ptmx inode here
> +	 */
> +	ihold(inode);
> +
>  	tty_add_file(tty, filp);
>  
>  	slave_inode = devpts_pty_new(inode,
> 

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

[toc] | [prev] | [next] | [standalone]

#1292422

On Tue, Dec 15, 2015 at 09:36:26AM -0800, Peter Hurley wrote:
> Hi Herton,
> 
> On 12/14/2015 07:29 PM, Herton R. Krzesinski wrote:
> > pty_unix98_shutdown allows a potential use after free of inode from
> > slave tty->driver_data: if final pty close is called with slave
> > tty_struct, and inode was released already by devpts_pty_kill at
> > pty_close, pty_unix98_shutdown will access stale data.
> 
> I'm not following this logic.
> 
> Suppose there is no open current tty alias for the slave pty; then
> there is one inode reference for the master and N + 1 inode references
> for however many times the slave has been opened.
> 
> If the master is closed first, the slave dentry is dropped so there
> are now N slave inode references. When the last slave closes, the
> slave inode is still valid when devpts_kill_index() is called.
> 
> So afaict, this problem only applies when /dev/tty is the final
> close, and at no other time. [Not to minimize the scale of the problem:
> quite often, /dev/tty would be the last file closed.]

Doh, you are right. The changelog is wrong here, and I must remove as what
I wrote isn't possible. The problem is only related to /dev/tty.

> 
> 
> > If the evicted inode is quickly reused again as another inode
> > instance, this can potentially break in the case the inode is on a
> > different devpts instance than the default devpts mount, not to
> > mention a possible ops if the inode_cache slab is destroyed and
> > reused as something else.
> > 
> > Also there is an evident problem in case the last close is from a
> > opened "/dev/tty" which points to the master/slave pty:
> 
> /dev/tty can only refer to the slave pty.

yes :(

> 
> 
> > since in this
> > case any of the tty->driver_data can be stale, due to all references/
> > files being closed before (files related to ptmx/pts inodes set at
> > tty->driver_data), we have the possibility of referencing an already
> > freed inode.
> 
> As I wrote above, I believe this is the only possible circumstance
> for which the file that is releasing could have stale pts inodes.
> 
> 
> > The fix here is to keep a reference on the opened master ptmx inode.
> > We maintain the inode referenced until the final pty_unix98_shutdown,
> > and only pass this inode to devpts_kill_index.
> 
> Let me think some on your proposed solution.

Ok, let me know what you think, at least I will have to repost the patch
with the changelog fixed, unless you think there is another/better solution
for the issue.

> 
> 
> > Signed-off-by: Herton R. Krzesinski <herton@redhat.com>
> > Cc: <stable@vger.kernel.org>
> 
> Afaict, the stable tag goes back to the original implementation.
> Did you research how far back the /dev/tty alias problem goes?

Hmm no. I did cc stable because the first report I got about this issue
was on RHEL 7 with 3.10 based kernel, so this issue goes far back
some releases that are still supported and similar code is there.

On a quick check on a 2.6.32 kernel, things were very different,
tty_release_dev() called directly devpts_kill_index with inode
from the same file being closed. I'll check more and adjust the tag.

> 
> Regards,
> Peter Hurley
> 
> > ---
> >  drivers/tty/pty.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
> > index a45660f..90743b0 100644
> > --- a/drivers/tty/pty.c
> > +++ b/drivers/tty/pty.c
> > @@ -681,7 +681,14 @@ static void pty_unix98_remove(struct tty_driver *driver, struct tty_struct *tty)
> >  /* this is called once with whichever end is closed last */
> >  static void pty_unix98_shutdown(struct tty_struct *tty)
> >  {
> > -	devpts_kill_index(tty->driver_data, tty->index);
> > +	struct inode *ptmx_inode;
> > +
> > +	if (tty->driver->subtype == PTY_TYPE_MASTER)
> > +		ptmx_inode = tty->driver_data;
> > +	else
> > +		ptmx_inode = tty->link->driver_data;
> > +	devpts_kill_index(ptmx_inode, tty->index);
> > +	iput(ptmx_inode); /* drop reference we acquired at ptmx_open */
> >  }
> >  
> >  static const struct tty_operations ptm_unix98_ops = {
> > @@ -773,6 +780,15 @@ static int ptmx_open(struct inode *inode, struct file *filp)
> >  	set_bit(TTY_PTY_LOCK, &tty->flags); /* LOCK THE SLAVE */
> >  	tty->driver_data = inode;
> >  
> > +	/*
> > +	 * In the rare case all references to ptmx inode are dropped (files
> > +	 * closed), and we still have a device opened pointing to the
> > +	 * master/slave pair (eg., "/dev/tty" opened), we must make sure that
> > +	 * the inode is still valid when we call the final pty_unix98_shutdown:
> > +	 * thus we must hold an additional reference to the ptmx inode here
> > +	 */
> > +	ihold(inode);
> > +
> >  	tty_add_file(tty, filp);
> >  
> >  	slave_inode = devpts_pty_new(inode,
> > 
> 

-- 
[]'s
Herton
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1292491

On Tue, Dec 15, 2015 at 04:05:09PM -0200, Herton R. Krzesinski wrote:
> On Tue, Dec 15, 2015 at 09:36:26AM -0800, Peter Hurley wrote:
> > 
> > 
> > > Signed-off-by: Herton R. Krzesinski <herton@redhat.com>
> > > Cc: <stable@vger.kernel.org>
> > 
> > Afaict, the stable tag goes back to the original implementation.
> > Did you research how far back the /dev/tty alias problem goes?
> 
> Hmm no. I did cc stable because the first report I got about this issue
> was on RHEL 7 with 3.10 based kernel, so this issue goes far back
> some releases that are still supported and similar code is there.
> 
> On a quick check on a 2.6.32 kernel, things were very different,
> tty_release_dev() called directly devpts_kill_index with inode
> from the same file being closed. I'll check more and adjust the tag.

FYI, checked here and the problem should start with 3.8, after commit
fa2ecfc5a68d85624bbd84f7d010860776b7e602 devpts_kill_index was moved
to pty.c/pty_unix98_shutdown

-- 
[]'s
Herton
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1292503

On 12/15/2015 11:23 AM, Herton R. Krzesinski wrote:
> On Tue, Dec 15, 2015 at 04:05:09PM -0200, Herton R. Krzesinski wrote:
>> On Tue, Dec 15, 2015 at 09:36:26AM -0800, Peter Hurley wrote:
>>>
>>>
>>>> Signed-off-by: Herton R. Krzesinski <herton@redhat.com>
>>>> Cc: <stable@vger.kernel.org>
>>>
>>> Afaict, the stable tag goes back to the original implementation.
>>> Did you research how far back the /dev/tty alias problem goes?
>>
>> Hmm no. I did cc stable because the first report I got about this issue
>> was on RHEL 7 with 3.10 based kernel, so this issue goes far back
>> some releases that are still supported and similar code is there.
>>
>> On a quick check on a 2.6.32 kernel, things were very different,
>> tty_release_dev() called directly devpts_kill_index with inode
>> from the same file being closed. I'll check more and adjust the tag.
> 
> FYI, checked here and the problem should start with 3.8, after commit
> fa2ecfc5a68d85624bbd84f7d010860776b7e602 devpts_kill_index was moved
> to pty.c/pty_unix98_shutdown
> 

istm this goes back to multi-instance devpts support added in 2.6.28.

Before then, there was no inode parameter because there was only
one devpts instance and the idas were global.

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

[toc] | [prev] | [next] | [standalone]

#1292525

On Tue, Dec 15, 2015 at 11:52:14AM -0800, Peter Hurley wrote:
> On 12/15/2015 11:23 AM, Herton R. Krzesinski wrote:
> > On Tue, Dec 15, 2015 at 04:05:09PM -0200, Herton R. Krzesinski wrote:
> >> On Tue, Dec 15, 2015 at 09:36:26AM -0800, Peter Hurley wrote:
> >>>
> >>>
> >>>> Signed-off-by: Herton R. Krzesinski <herton@redhat.com>
> >>>> Cc: <stable@vger.kernel.org>
> >>>
> >>> Afaict, the stable tag goes back to the original implementation.
> >>> Did you research how far back the /dev/tty alias problem goes?
> >>
> >> Hmm no. I did cc stable because the first report I got about this issue
> >> was on RHEL 7 with 3.10 based kernel, so this issue goes far back
> >> some releases that are still supported and similar code is there.
> >>
> >> On a quick check on a 2.6.32 kernel, things were very different,
> >> tty_release_dev() called directly devpts_kill_index with inode
> >> from the same file being closed. I'll check more and adjust the tag.
> > 
> > FYI, checked here and the problem should start with 3.8, after commit
> > fa2ecfc5a68d85624bbd84f7d010860776b7e602 devpts_kill_index was moved
> > to pty.c/pty_unix98_shutdown
> > 
> 
> istm this goes back to multi-instance devpts support added in 2.6.28.
> 
> Before then, there was no inode parameter because there was only
> one devpts instance and the idas were global.

Yeah, I'm not ruling out problems with devpts instances prior to 3.8, where to
me the wrong inode will be given in the final close with /dev/tty case, when the
ptmx is on a different instance other than the main ptmx instance (
pts_sb_from_inode will choose the "root"/main devpts instance, as the /dev/tty
inode usually is inode tied to devtmpfs mount at /dev). Both fa2ecfc5a68d85624b
and the new fix could be backported to 3.7 and as far as 2.6.28 perhaps, not
sure if anything else will be needed, however may not be worth the trouble.

-- 
[]'s
Herton
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1292527

On 12/15/2015 12:34 PM, Herton R. Krzesinski wrote:
> On Tue, Dec 15, 2015 at 11:52:14AM -0800, Peter Hurley wrote:
>> On 12/15/2015 11:23 AM, Herton R. Krzesinski wrote:
>>> On Tue, Dec 15, 2015 at 04:05:09PM -0200, Herton R. Krzesinski wrote:
>>>> On Tue, Dec 15, 2015 at 09:36:26AM -0800, Peter Hurley wrote:
>>>>>
>>>>>
>>>>>> Signed-off-by: Herton R. Krzesinski <herton@redhat.com>
>>>>>> Cc: <stable@vger.kernel.org>
>>>>>
>>>>> Afaict, the stable tag goes back to the original implementation.
>>>>> Did you research how far back the /dev/tty alias problem goes?
>>>>
>>>> Hmm no. I did cc stable because the first report I got about this issue
>>>> was on RHEL 7 with 3.10 based kernel, so this issue goes far back
>>>> some releases that are still supported and similar code is there.
>>>>
>>>> On a quick check on a 2.6.32 kernel, things were very different,
>>>> tty_release_dev() called directly devpts_kill_index with inode
>>>> from the same file being closed. I'll check more and adjust the tag.
>>>
>>> FYI, checked here and the problem should start with 3.8, after commit
>>> fa2ecfc5a68d85624bbd84f7d010860776b7e602 devpts_kill_index was moved
>>> to pty.c/pty_unix98_shutdown
>>>
>>
>> istm this goes back to multi-instance devpts support added in 2.6.28.
>>
>> Before then, there was no inode parameter because there was only
>> one devpts instance and the idas were global.
> 
> Yeah, I'm not ruling out problems with devpts instances prior to 3.8, where to
> me the wrong inode will be given in the final close with /dev/tty case, when the
> ptmx is on a different instance other than the main ptmx instance (
> pts_sb_from_inode will choose the "root"/main devpts instance, as the /dev/tty
> inode usually is inode tied to devtmpfs mount at /dev). Both fa2ecfc5a68d85624b
> and the new fix could be backported to 3.7 and as far as 2.6.28 perhaps, not
> sure if anything else will be needed, however may not be worth the trouble.

I think a 2.6.28 tag is sufficient.

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

[toc] | [prev] | [next] | [standalone]

#1299064

On Tue, Dec 15, 2015 at 04:05:09PM -0200, Herton R. Krzesinski wrote:
> On Tue, Dec 15, 2015 at 09:36:26AM -0800, Peter Hurley wrote:
> > > since in this
> > > case any of the tty->driver_data can be stale, due to all references/
> > > files being closed before (files related to ptmx/pts inodes set at
> > > tty->driver_data), we have the possibility of referencing an already
> > > freed inode.
> > 
> > As I wrote above, I believe this is the only possible circumstance
> > for which the file that is releasing could have stale pts inodes.
> > 
> > 
> > > The fix here is to keep a reference on the opened master ptmx inode.
> > > We maintain the inode referenced until the final pty_unix98_shutdown,
> > > and only pass this inode to devpts_kill_index.
> > 
> > Let me think some on your proposed solution.
> 
> Ok, let me know what you think, at least I will have to repost the patch
> with the changelog fixed, unless you think there is another/better solution
> for the issue.

Hi Peter, any news on this issue?

I gave some more thought and testing into this, and I think we simply should do
a change like below instead of my previous patch proposal:

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index a45660f..73e36bd 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -68,6 +68,7 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
 			mutex_lock(&devpts_mutex);
 			if (tty->link->driver_data)
 				devpts_pty_kill(tty->link->driver_data);
+			devpts_kill_index(tty->driver_data, tty->index);
 			mutex_unlock(&devpts_mutex);
 		}
 #endif
@@ -678,12 +679,6 @@ static void pty_unix98_remove(struct tty_driver *driver, struct tty_struct *tty)
 {
 }
 
-/* this is called once with whichever end is closed last */
-static void pty_unix98_shutdown(struct tty_struct *tty)
-{
-	devpts_kill_index(tty->driver_data, tty->index);
-}
-
 static const struct tty_operations ptm_unix98_ops = {
 	.lookup = ptm_unix98_lookup,
 	.install = pty_unix98_install,
@@ -697,7 +692,6 @@ static const struct tty_operations ptm_unix98_ops = {
 	.unthrottle = pty_unthrottle,
 	.ioctl = pty_unix98_ioctl,
 	.resize = pty_resize,
-	.shutdown = pty_unix98_shutdown,
 	.cleanup = pty_cleanup
 };
 
@@ -715,7 +709,6 @@ static const struct tty_operations pty_unix98_ops = {
 	.set_termios = pty_set_termios,
 	.start = pty_start,
 	.stop = pty_stop,
-	.shutdown = pty_unix98_shutdown,
 	.cleanup = pty_cleanup,
 };
 
-- 
2.4.3


That is, move devpts_kill_index up into pty_close(). It also resolves the
problem, while at the same time handles another problem which my previous
patch didn't catch, for example look at this other test case:

#define _XOPEN_SOURCE
#include <fcntl.h>
#include <stdlib.h>
#include <sys/ioctl.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>

int main(int argc, char **argv)
{
        pid_t pid;
        int ptm_fd, pty_fd, tty_fd;

        system("mkdir -p /mnt/newpts");
        system("mount -t devpts -o newinstance none /mnt/newpts");
        pid = fork();
        if (pid != 0)
                exit(0);
        daemon(1, 0);
        ptm_fd = open("/mnt/newpts/ptmx", O_RDWR);
        unlockpt(ptm_fd);
        pty_fd = open("/mnt/newpts/0", O_RDWR);
        tty_fd = open("/dev/tty", O_RDWR);
        pid = fork();
        if (pid == 0) {
                ioctl(tty_fd, TIOCNOTTY, NULL);
                setsid();
                sleep(20);
                close(pty_fd);
                close(ptm_fd);
                system("umount /mnt/newpts");
                sleep(10);
                exit(0);
        }
        sleep(10);
        return 0;
}

The idea here is to umount a pts mount while still we have /dev/tty pointing to
a pty opened...

And of course it doesn't go well with the late devpts_kill_index:

[ 1326.233991] ------------[ cut here ]------------
[ 1326.234014] WARNING: CPU: 1 PID: 2668 at lib/idr.c:1051 ida_remove+0x9b/0x130()
[ 1326.234015] ida_remove called for id=0 which is not allocated.
[ 1326.234016] Modules linked in: 8021q mrp garp stp llc nf_conntrack_ipv4 nf_defrag_ipv4 ip6t_REJECT nf_reject_ipv6 nf_conntrack_ipv6 nf_defrag_ipv6 xt_state nf_conntrack ip6table_filter ip6_tables binfmt_misc ppdev joydev floppy parport_pc parport serio_raw tpm_tis tpm virtio_balloon virtio_console virtio_net iosf_mbi crct10dif_pclmul crc32_pclmul pcspkr snd_hda_codec_generic i2c_piix4 snd_hda_intel snd_hda_codec snd_hda_core snd_hwdep snd_seq snd_seq_device snd_pcm snd_timer snd soundcore qxl ttm drm_kms_helper drm virtio_blk crc32c_intel virtio_pci virtio_ring virtio pata_acpi ata_generic [last unloaded: speedstep_lib]
[ 1326.234061] CPU: 1 PID: 2668 Comm: newpty Not tainted 4.4.0-rc7 #1
[ 1326.234062] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.8.1-20150318_183358- 04/01/2014
[ 1326.234065]  000000000000041b ffff8800375ffbc8 ffffffff8139aed4 0000000000000009
[ 1326.234068]  ffff8800375ffc18 000000000000041b ffff8800375ffc18 ffff8800375ffc08
[ 1326.234069]  ffffffff81096f15 ffff8800375ffbf8 ffff8801399c47e0 0000000000000000
[ 1326.234071] Call Trace:
[ 1326.234083]  [<ffffffff8139aed4>] dump_stack+0x48/0x64
[ 1326.234092]  [<ffffffff81096f15>] warn_slowpath_common+0x95/0xe0
[ 1326.234094]  [<ffffffff81097016>] warn_slowpath_fmt+0x46/0x50
[ 1326.234104]  [<ffffffff811fc292>] ? kfree+0x112/0x150
[ 1326.234105]  [<ffffffff8139c75b>] ida_remove+0x9b/0x130
[ 1326.234111]  [<ffffffff8129f3c7>] devpts_kill_index+0x57/0x80
[ 1326.234120]  [<ffffffff81480338>] pty_unix98_shutdown+0x18/0x20
[ 1326.234124]  [<ffffffff81474a2e>] release_tty+0x3e/0xe0
[ 1326.234129]  [<ffffffff8177d476>] ? mutex_lock+0x16/0x40
[ 1326.234131]  [<ffffffff81475b0a>] tty_release+0x44a/0x580
[ 1326.234135]  [<ffffffff8121e3d5>] __fput+0xb5/0x200
[ 1326.234137]  [<ffffffff8121e5ce>] ____fput+0xe/0x10
[ 1326.234143]  [<ffffffff810b2eb8>] task_work_run+0x68/0xa0
[ 1326.234145]  [<ffffffff8109a3e0>] do_exit+0x320/0x670
[ 1326.234147]  [<ffffffff810673d4>] ? __do_page_fault+0x1a4/0x450
[ 1326.234155]  [<ffffffff811361d0>] ? __audit_syscall_entry+0xb0/0x110
[ 1326.234161]  [<ffffffff81003376>] ? do_audit_syscall_entry+0x66/0x70
[ 1326.234164]  [<ffffffff8109a781>] do_group_exit+0x51/0xc0
[ 1326.234166]  [<ffffffff8109a807>] SyS_exit_group+0x17/0x20
[ 1326.234169]  [<ffffffff8177f92e>] entry_SYSCALL_64_fastpath+0x12/0x71
[ 1326.234171] ---[ end trace 23cebcbb1a28e0e8 ]---

So to avoid all this problem/headache with the late devpts_kill_index, we should
just move devpts_kill_index up into pty_close I think. I don't see any problem
with this yet.

If you are ok I would like to submit the new diff/patch above with a proper
changelog/signoff etc.

-- 
[]'s
Herton
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web