Re: [PATCH 3.16 194/366] drivers: tty: Merge alloc_tty_struct and initialize_tty_struct

[Ben Hutchings]

On Sun, 2018-10-14 at 19:22 +0200, Rasmus Villemoes wrote:
> IIRC, I messed up back then, so you'll need some followup as well, but I'm
> on my phone ATM.

I guess that's commit 07584d4a356e "drivers: tty: Fix use-after-free in
pty_common_install"?  I missed that but will add it now.

>  I assume you're taking this to make it easier to backport
> some actual fix?

This is required as preparation for commit 903f9db10f18 "tty: Don't
call panic() at tty_ldisc_init()".

Ben.

> 
> On Sun, Oct 14, 2018, 17:39 Ben Hutchings <ben@xxxxxxxxxxxxxxx> wrote:
> 
> > 3.16.60-rc1 review patch.  If anyone has any objections, please let me
> > know.
> > 
> > ------------------
> > 
> > From: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
> > 
> > commit 2c964a2f4191f2229566895f1a0e85f8339f5dd1 upstream.
> > 
> > The two functions alloc_tty_struct and initialize_tty_struct are
> > always called together. Merge them into alloc_tty_struct, updating its
> > prototype and the only two callers of these functions.
> > 
> > Signed-off-by: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/tty/pty.c    | 19 +++++++++----------
> >  drivers/tty/tty_io.c | 37 +++++++++++++------------------------
> >  include/linux/tty.h  |  4 +---
> >  3 files changed, 23 insertions(+), 37 deletions(-)
> > 
> > --- a/drivers/tty/pty.c
> > +++ b/drivers/tty/pty.c
> > @@ -319,7 +319,7 @@ done:
> >   *     pty_common_install              -       set up the pty pair
> >   *     @driver: the pty driver
> >   *     @tty: the tty being instantiated
> > - *     @bool: legacy, true if this is BSD style
> > + *     @legacy: true if this is BSD style
> >   *
> >   *     Perform the initial set up for the tty/pty pair. Called from the
> >   *     tty layer when the port is first opened.
> > @@ -334,18 +334,17 @@ static int pty_common_install(struct tty
> >         int idx = tty->index;
> >         int retval = -ENOMEM;
> > 
> > -       o_tty = alloc_tty_struct();
> > -       if (!o_tty)
> > -               goto err;
> >         ports[0] = kmalloc(sizeof **ports, GFP_KERNEL);
> >         ports[1] = kmalloc(sizeof **ports, GFP_KERNEL);
> >         if (!ports[0] || !ports[1])
> > -               goto err_free_tty;
> > +               goto err;
> >         if (!try_module_get(driver->other->owner)) {
> >                 /* This cannot in fact currently happen */
> > -               goto err_free_tty;
> > +               goto err;
> >         }
> > -       initialize_tty_struct(o_tty, driver->other, idx);
> > +       o_tty = alloc_tty_struct(driver->other, idx);
> > +       if (!o_tty)
> > +               goto err_put_module;
> > 
> >         if (legacy) {
> >                 /* We always use new tty termios data so we can do this
> > @@ -390,12 +389,12 @@ err_free_termios:
> >                 tty_free_termios(tty);
> >  err_deinit_tty:
> >         deinitialize_tty_struct(o_tty);
> > +       free_tty_struct(o_tty);
> > +err_put_module:
> >         module_put(o_tty->driver->owner);
> > -err_free_tty:
> > +err:
> >         kfree(ports[0]);
> >         kfree(ports[1]);
> > -       free_tty_struct(o_tty);
> > -err:
> >         return retval;
> >  }
> > 
> > --- a/drivers/tty/tty_io.c
> > +++ b/drivers/tty/tty_io.c
> > @@ -157,20 +157,6 @@ static void __proc_set_tty(struct task_s
> >  static void proc_set_tty(struct task_struct *tsk, struct tty_struct *tty);
> > 
> >  /**
> > - *     alloc_tty_struct        -       allocate a tty object
> > - *
> > - *     Return a new empty tty structure. The data fields have not
> > - *     been initialized in any way but has been zeroed
> > - *
> > - *     Locking: none
> > - */
> > -
> > -struct tty_struct *alloc_tty_struct(void)
> > -{
> > -       return kzalloc(sizeof(struct tty_struct), GFP_KERNEL);
> > -}
> > -
> > -/**
> >   *     free_tty_struct         -       free a disused tty
> >   *     @tty: tty struct to free
> >   *
> > @@ -1455,12 +1441,11 @@ struct tty_struct *tty_init_dev(struct t
> >         if (!try_module_get(driver->owner))
> >                 return ERR_PTR(-ENODEV);
> > 
> > -       tty = alloc_tty_struct();
> > +       tty = alloc_tty_struct(driver, idx);
> >         if (!tty) {
> >                 retval = -ENOMEM;
> >                 goto err_module_put;
> >         }
> > -       initialize_tty_struct(tty, driver, idx);
> > 
> >         tty_lock(tty);
> >         retval = tty_driver_install_tty(driver, tty);
> > @@ -3034,19 +3019,21 @@ static struct device *tty_get_device(str
> > 
> > 
> >  /**
> > - *     initialize_tty_struct
> > - *     @tty: tty to initialize
> > + *     alloc_tty_struct
> >   *
> > - *     This subroutine initializes a tty structure that has been newly
> > - *     allocated.
> > + *     This subroutine allocates and initializes a tty structure.
> >   *
> > - *     Locking: none - tty in question must not be exposed at this point
> > + *     Locking: none - tty in question is not exposed at this point
> >   */
> > 
> > -void initialize_tty_struct(struct tty_struct *tty,
> > -               struct tty_driver *driver, int idx)
> > +struct tty_struct *alloc_tty_struct(struct tty_driver *driver, int idx)
> >  {
> > -       memset(tty, 0, sizeof(struct tty_struct));
> > +       struct tty_struct *tty;
> > +
> > +       tty = kzalloc(sizeof(*tty), GFP_KERNEL);
> > +       if (!tty)
> > +               return NULL;
> > +
> >         kref_init(&tty->kref);
> >         tty->magic = TTY_MAGIC;
> >         tty_ldisc_init(tty);
> > @@ -3070,6 +3057,8 @@ void initialize_tty_struct(struct tty_st
> >         tty->index = idx;
> >         tty_line_name(driver, idx, tty->name);
> >         tty->dev = tty_get_device(tty);
> > +
> > +       return tty;
> >  }
> > 
> >  /**
> > --- a/include/linux/tty.h
> > +++ b/include/linux/tty.h
> > @@ -477,13 +477,11 @@ extern int tty_mode_ioctl(struct tty_str
> >                         unsigned int cmd, unsigned long arg);
> >  extern int tty_perform_flush(struct tty_struct *tty, unsigned long arg);
> >  extern void tty_default_fops(struct file_operations *fops);
> > -extern struct tty_struct *alloc_tty_struct(void);
> > +extern struct tty_struct *alloc_tty_struct(struct tty_driver *driver, int
> > idx);
> >  extern int tty_alloc_file(struct file *file);
> >  extern void tty_add_file(struct tty_struct *tty, struct file *file);
> >  extern void tty_free_file(struct file *file);
> >  extern void free_tty_struct(struct tty_struct *tty);
> > -extern void initialize_tty_struct(struct tty_struct *tty,
> > -               struct tty_driver *driver, int idx);
> >  extern void deinitialize_tty_struct(struct tty_struct *tty);
> >  extern struct tty_struct *tty_init_dev(struct tty_driver *driver, int
> > idx);
> >  extern int tty_release(struct inode *inode, struct file *filp);
> > 
> > 
-- 
Ben Hutchings
I haven't lost my mind; it's backed up on tape somewhere.

Attachment: signature.asc
Description: This is a digitally signed message part