Alan Cox wrote:
>
> > > drivers and fix their open/close routines to work with this patch? Peter
> > > and I can take some time to do that--if that would help.
> >
> > That would be one big help. Having done that I'd like to go over it all with
> > Ted first (if he has time) before I push it to Linus
>
> So I stuck my justify this change to Ted hat on. And failed.
>
> For one the cleanest way to handle all the locking is to propogate the other
> locking fix styles into both the ldisc and serial drivers. At least for 2.4
> until the 2.5 folks get their deep magic inactivity based clean up working
>
> The advantage of doing that is that modules that do play with use counts will
> not do anything worse than they do now, and will remain fully compatible.
>
> The ldisc race is also real and completely unfixed right now.
>
I have a work-in-non-progress here which addresses a few of
these things. It would be good if someone could review it
and finish it off...
- implements tty->ldisc_sem to plug race between do_tty_hangup()
and tty_set_ldisc(). Is this the ldisc race to which you refer?
- implements the `struct module *owner' in tty_driver and
tty_ldisc. Manipulates this in all the right places.
- Actually *uses* ->owner in ppp_async.c, n_r3964.c and serial.c
Other tty and ldisc drivers need to be changed to set ->owner.
There's a fight between n_hdlc.c and n_r3964.c which hasn't been fixed
yet. If you attach r3964 to a tty and then detach it, it leaves
a non-zero value in tty->disc_data. This then prevents you from being
able to attach n_hdlc to the tty, because it checks for null ->disc_data.
Best fix for this is to make sure all the ldisc close routines zero out
disc_data when they're finished.
--- linux-2.4.2-ac24/include/linux/tty_driver.h Tue Jan 30 18:24:56 2001
+++ ac/include/linux/tty_driver.h Sun Mar 25 21:09:30 2001
@@ -117,6 +117,14 @@
#include <linux/fs.h>
+#ifdef CONFIG_MODULES
+#include <linux/module.h>
+#define SET_TTY_OWNER(driver) \
+ do { (driver)->owner = THIS_MODULE; } while (0)
+#define SET_LDISC_OWNER(ldisc) \
+ do { (ldisc)->owner = THIS_MODULE; } while (0)
+#endif
+
struct tty_driver {
int magic; /* magic number for this structure */
const char *driver_name;
@@ -176,6 +184,9 @@
*/
struct tty_driver *next;
struct tty_driver *prev;
+#ifdef CONFIG_MODULES
+ struct module *owner;
+#endif
};
/* tty driver magic number */
--- linux-2.4.2-ac24/include/linux/tty_ldisc.h Sat Mar 24 14:28:24 2001
+++ ac/include/linux/tty_ldisc.h Sun Mar 25 21:09:30 2001
@@ -100,6 +100,8 @@
#include <linux/fs.h>
#include <linux/wait.h>
+struct module;
+
struct tty_ldisc {
int magic;
char *name;
@@ -129,6 +131,9 @@
char *fp, int count);
int (*receive_room)(struct tty_struct *);
void (*write_wakeup)(struct tty_struct *);
+#ifdef CONFIG_MODULES
+ struct module *owner;
+#endif
};
#define TTY_LDISC_MAGIC 0x5403
--- linux-2.4.2-ac24/include/linux/tty.h Sat Mar 24 14:28:24 2001
+++ ac/include/linux/tty.h Sun Mar 25 21:09:30 2001
@@ -306,6 +306,7 @@
unsigned int canon_column;
struct semaphore atomic_read;
struct semaphore atomic_write;
+ struct semaphore ldisc_sem; /* Lock this while we're manipulating ldisc */
spinlock_t read_lock;
/* If the tty has a pending do_SAK, queue it here - akpm */
struct tq_struct SAK_tq;
--- linux-2.4.2-ac24/drivers/char/tty_io.c Sat Mar 24 14:28:05 2001
+++ ac/drivers/char/tty_io.c Sun Mar 25 23:33:45 2001
@@ -110,10 +110,10 @@
#endif
extern int rio_init(void);
-#define CONSOLE_DEV MKDEV(TTY_MAJOR,0)
-#define TTY_DEV MKDEV(TTYAUX_MAJOR,0)
-#define SYSCONS_DEV MKDEV(TTYAUX_MAJOR,1)
-#define PTMX_DEV MKDEV(TTYAUX_MAJOR,2)
+#define CONSOLE_DEV MKDEV(TTY_MAJOR,0) /* /dev/systty */
+#define TTY_DEV MKDEV(TTYAUX_MAJOR,0) /* /dev/tty */
+#define SYSCONS_DEV MKDEV(TTYAUX_MAJOR,1) /* /dev/console */
+#define PTMX_DEV MKDEV(TTYAUX_MAJOR,2) /* /dev/ptmx */
#undef TTY_DEBUG_HANGUP
@@ -188,6 +188,65 @@
}
/*
+ * Lock a driver's module into core and increment its low-level refcount.
+ * Return 0 on success. If we return non-zero then the driver module isn't there
+ * any more and action needs to be taken by the caller
+ */
+static int tty_dev_hold(struct tty_driver *driver)
+{
+#ifdef CONFIG_MODULES
+ if (try_inc_mod_count(driver->owner) == 0)
+ return -ENODEV; /* Module is deleted */
+#endif
+ (*driver->refcount)++;
+ return 0;
+}
+
+/*
+ * Release the driver and (possibly) its module
+ */
+static void tty_dev_put(struct tty_driver *driver)
+{
+#ifdef CONFIG_MODULES
+ if (driver->owner)
+ __MOD_DEC_USE_COUNT(driver->owner);
+#endif
+ (*driver->refcount)--;
+}
+
+/*
+ * Lock an ldisc's module into core. Return non-zero if the
+ * containing module is unusable - remedial action is needed
+ * by the caller
+ */
+static int tty_ldisc_hold(struct tty_ldisc *ldisc)
+{
+#ifdef CONFIG_MODULES
+ if (try_inc_mod_count(ldisc->owner) == 0)
+ return -ENODEV; /* Module is deleted */
+ return 0;
+#endif
+}
+
+/*
+ * Release an ldisc and (possibly) its module
+ */
+static void tty_ldisc_put(struct tty_ldisc *ldisc)
+{
+#ifdef CONFIG_MODULES
+ if (ldisc->owner) {
+ int uc;
+ __MOD_DEC_USE_COUNT(ldisc->owner);
+ uc = atomic_read(&(ldisc->owner)->uc.usecount);
+ if (uc < 0) {
+ printk("tty_ldisc_put: count=%d. This is not good\n",
+ atomic_read(&(ldisc->owner)->uc.usecount));
+ }
+ }
+#endif
+}
+
+/*
* This routine returns the name of tty.
*/
static char *
@@ -276,12 +335,15 @@
/* Set the discipline of a tty line. */
static int tty_set_ldisc(struct tty_struct *tty, int ldisc)
{
- int retval = 0;
+ int retval;
struct tty_ldisc o_ldisc;
char buf[64];
- if ((ldisc < N_TTY) || (ldisc >= NR_LDISCS))
- return -EINVAL;
+ down(&tty->ldisc_sem); /* We're changing the ldisc. Get exclusive access */
+ if ((ldisc < N_TTY) || (ldisc >= NR_LDISCS)) {
+ retval = -EINVAL;
+ goto out;
+ }
/* Eduardo Blanco <[EMAIL PROTECTED]> */
/* Cyrus Durgin <[EMAIL PROTECTED]> */
if (!(ldiscs[ldisc].flags & LDISC_FLAG_DEFINED)) {
@@ -289,11 +351,14 @@
sprintf(modname, "tty-ldisc-%d", ldisc);
request_module (modname);
}
- if (!(ldiscs[ldisc].flags & LDISC_FLAG_DEFINED))
- return -EINVAL;
-
- if (tty->ldisc.num == ldisc)
- return 0; /* We are already in the desired discipline */
+ if (!(ldiscs[ldisc].flags & LDISC_FLAG_DEFINED)) {
+ retval = -EINVAL;
+ goto out;
+ }
+ if (tty->ldisc.num == ldisc) {
+ retval = 0; /* We are already in the desired discipline */
+ goto out;
+ }
o_ldisc = tty->ldisc;
tty_wait_until_sent(tty, 0);
@@ -302,15 +367,24 @@
if (tty->ldisc.close)
(tty->ldisc.close)(tty);
+ /* Don't do tty_ldisc_put(&o_ldisc) - we may need it later */
+
/* Now set up the new line discipline. */
tty->ldisc = ldiscs[ldisc];
tty->termios->c_line = ldisc;
- if (tty->ldisc.open)
+
+ retval = tty_ldisc_hold(&tty->ldisc);
+
+ if (retval == 0 && tty->ldisc.open)
retval = (tty->ldisc.open)(tty);
+
if (retval < 0) {
+ tty_ldisc_put(&tty->ldisc);
+ tty_ldisc_hold(&o_ldisc);
tty->ldisc = o_ldisc;
tty->termios->c_line = tty->ldisc.num;
if (tty->ldisc.open && (tty->ldisc.open(tty) < 0)) {
+ tty_ldisc_put(&tty->ldisc);
tty->ldisc = ldiscs[N_TTY];
tty->termios->c_line = N_TTY;
if (tty->ldisc.open) {
@@ -323,8 +397,11 @@
}
}
}
+ tty_ldisc_put(&o_ldisc); /* Do it now */
if (tty->ldisc.num != o_ldisc.num && tty->driver.set_ldisc)
tty->driver.set_ldisc(tty);
+out:
+ up(&tty->ldisc_sem);
return retval;
}
@@ -466,8 +543,9 @@
filp->f_op = &hung_up_tty_fops;
}
file_list_unlock();
-
- /* FIXME! What are the locking issues here? This may me overdoing things.. */
+
+ /* Pin down tty->ldisc to avoid racing with tty_set_ldisc */
+ down(&tty->ldisc_sem);
{
unsigned long flags;
@@ -494,6 +572,7 @@
if (tty->ldisc.num != ldiscs[N_TTY].num) {
if (tty->ldisc.close)
(tty->ldisc.close)(tty);
+ tty_ldisc_put(&tty->ldisc);
tty->ldisc = ldiscs[N_TTY];
tty->termios->c_line = N_TTY;
if (tty->ldisc.open) {
@@ -503,6 +582,7 @@
-i);
}
}
+ up(&tty->ldisc_sem);
read_lock(&tasklist_lock);
for_each_task(p) {
@@ -907,7 +987,8 @@
*o_ltp_loc = o_ltp;
o_tty->termios = *o_tp_loc;
o_tty->termios_locked = *o_ltp_loc;
- (*driver->other->refcount)++;
+ if (tty_dev_hold(driver->other) != 0)
+ goto free_mem_out;
if (driver->subtype == PTY_TYPE_MASTER)
o_tty->count++;
@@ -916,6 +997,9 @@
o_tty->link = tty;
}
+ if (tty_dev_hold(driver) != 0)
+ goto free_mem_out;
+
/*
* All structures have been allocated, so now we install them.
* Failures after this point use release_mem to clean up, so
@@ -929,7 +1013,6 @@
*ltp_loc = ltp;
tty->termios = *tp_loc;
tty->termios_locked = *ltp_loc;
- (*driver->refcount)++;
tty->count++;
/*
@@ -1026,7 +1109,7 @@
kfree(tp);
}
o_tty->magic = 0;
- (*o_tty->driver.refcount)--;
+ tty_dev_put(&o_tty->driver);
free_tty_struct(o_tty);
}
@@ -1037,7 +1120,7 @@
kfree(tp);
}
tty->magic = 0;
- (*tty->driver.refcount)--;
+ tty_dev_put(&tty->driver);
free_tty_struct(tty);
}
@@ -1254,11 +1337,13 @@
*/
if (tty->ldisc.close)
(tty->ldisc.close)(tty);
+ tty_ldisc_put(&tty->ldisc);
tty->ldisc = ldiscs[N_TTY];
tty->termios->c_line = N_TTY;
if (o_tty) {
if (o_tty->ldisc.close)
(o_tty->ldisc.close)(o_tty);
+ tty_ldisc_put(&o_tty->ldisc);
o_tty->ldisc = ldiscs[N_TTY];
}
@@ -1299,21 +1384,21 @@
retry_open:
noctty = filp->f_flags & O_NOCTTY;
device = inode->i_rdev;
- if (device == TTY_DEV) {
+ if (device == TTY_DEV) { /* /dev/tty */
if (!current->tty)
return -ENXIO;
device = current->tty->device;
- filp->f_flags |= O_NONBLOCK; /* Don't let /dev/tty block */
+ filp->f_flags |= O_NONBLOCK; /* Don't let /dev/tty block */
/* noctty = 1; */
}
#ifdef CONFIG_VT
- if (device == CONSOLE_DEV) {
+ if (device == CONSOLE_DEV) { /* /dev/systty */
extern int fg_console;
device = MKDEV(TTY_MAJOR, fg_console + 1);
noctty = 1;
}
#endif
- if (device == SYSCONS_DEV) {
+ if (device == SYSCONS_DEV) { /* /dev/console */
struct console *c = console_drivers;
while(c && !c->device)
c = c->next;
@@ -1324,7 +1409,7 @@
noctty = 1;
}
- if (device == PTMX_DEV) {
+ if (device == PTMX_DEV) { /* /dev/ptmx */
#ifdef CONFIG_UNIX98_PTYS
/* find a free pty. */
@@ -1996,6 +2081,7 @@
tty->tq_hangup.data = tty;
sema_init(&tty->atomic_read, 1);
sema_init(&tty->atomic_write, 1);
+ sema_init(&tty->ldisc_sem, 1);
spin_lock_init(&tty->read_lock);
INIT_LIST_HEAD(&tty->tty_files);
INIT_TQUEUE(&tty->SAK_tq, 0, 0);
--- linux-2.4.2-ac24/drivers/char/serial.c Sat Mar 24 14:28:04 2001
+++ ac/drivers/char/serial.c Sun Mar 25 21:09:30 2001
@@ -212,6 +212,14 @@
#include <linux/sysrq.h>
#endif
+#ifdef SET_TTY_OWNER
+#define TTY_MOD_INC do {} while (0)
+#define TTY_MOD_DEC do {} while (0)
+#else
+#define TTY_MOD_INC MOD_INC_USE_COUNT
+#define TTY_MOD_DEC MOD_DEC_USE_COUNT
+#endif
+
/*
* All of the compatibilty code so we can compile serial.c against
* older kernels is hidden in serial_compat.h
@@ -2761,7 +2769,7 @@
if (tty_hung_up_p(filp)) {
DBG_CNT("before DEC-hung");
- MOD_DEC_USE_COUNT;
+ TTY_MOD_DEC;
restore_flags(flags);
return;
}
@@ -2788,7 +2796,7 @@
}
if (state->count) {
DBG_CNT("before DEC-2");
- MOD_DEC_USE_COUNT;
+ TTY_MOD_DEC;
restore_flags(flags);
return;
}
@@ -2844,7 +2852,7 @@
info->flags &= ~(ASYNC_NORMAL_ACTIVE|ASYNC_CALLOUT_ACTIVE|
ASYNC_CLOSING);
wake_up_interruptible(&info->close_wait);
- MOD_DEC_USE_COUNT;
+ TTY_MOD_DEC;
}
/*
@@ -3128,21 +3136,21 @@
int retval, line;
unsigned long page;
- MOD_INC_USE_COUNT;
+ TTY_MOD_INC;
line = MINOR(tty->device) - tty->driver.minor_start;
if ((line < 0) || (line >= NR_PORTS)) {
- MOD_DEC_USE_COUNT;
+ TTY_MOD_DEC;
return -ENODEV;
}
retval = get_async_struct(line, &info);
if (retval) {
- MOD_DEC_USE_COUNT;
+ TTY_MOD_DEC;
return retval;
}
tty->driver_data = info;
info->tty = tty;
if (serial_paranoia_check(info, tty->device, "rs_open")) {
- MOD_DEC_USE_COUNT;
+ TTY_MOD_DEC;
return -ENODEV;
}
@@ -3157,7 +3165,7 @@
if (!tmp_buf) {
page = get_zeroed_page(GFP_KERNEL);
if (!page) {
- MOD_DEC_USE_COUNT;
+ TTY_MOD_DEC;
return -ENOMEM;
}
if (tmp_buf)
@@ -3173,7 +3181,7 @@
(info->flags & ASYNC_CLOSING)) {
if (info->flags & ASYNC_CLOSING)
interruptible_sleep_on(&info->close_wait);
- MOD_DEC_USE_COUNT;
+ TTY_MOD_DEC;
#ifdef SERIAL_DO_RESTART
return ((info->flags & ASYNC_HUP_NOTIFY) ?
-EAGAIN : -ERESTARTSYS);
@@ -3187,7 +3195,7 @@
*/
retval = startup(info);
if (retval) {
- MOD_DEC_USE_COUNT;
+ TTY_MOD_DEC;
return retval;
}
@@ -3197,7 +3205,7 @@
printk("rs_open returning after block_til_ready with %d\n",
retval);
#endif
- MOD_DEC_USE_COUNT;
+ TTY_MOD_DEC;
return retval;
}
@@ -5180,7 +5188,9 @@
serial_driver.wait_until_sent = rs_wait_until_sent;
serial_driver.read_proc = rs_read_proc;
#endif
-
+#ifdef SET_TTY_OWNER
+ SET_TTY_OWNER(&serial_driver);
+#endif
/*
* The callout device is just like normal device except for
* major number and the subtype code.
@@ -5413,12 +5423,16 @@
del_timer_sync(&serial_timer);
save_flags(flags); cli();
remove_bh(SERIAL_BH);
- if ((e1 = tty_unregister_driver(&serial_driver)))
+ if ((e1 = tty_unregister_driver(&serial_driver))) {
printk("serial: failed to unregister serial driver (%d)\n",
e1);
- if ((e2 = tty_unregister_driver(&callout_driver)))
+ BUG();
+ }
+ if ((e2 = tty_unregister_driver(&callout_driver))) {
printk("serial: failed to unregister callout driver (%d)\n",
e2);
+ BUG();
+ }
restore_flags(flags);
for (i = 0; i < NR_PORTS; i++) {
--- linux-2.4.2-ac24/drivers/net/ppp_async.c Sat Mar 24 14:28:11 2001
+++ ac/drivers/net/ppp_async.c Sun Mar 25 21:09:30 2001
@@ -113,7 +113,6 @@
struct asyncppp *ap;
int err;
- MOD_INC_USE_COUNT;
err = -ENOMEM;
ap = kmalloc(sizeof(*ap), GFP_KERNEL);
if (ap == 0)
@@ -146,7 +145,6 @@
out_free:
kfree(ap);
out:
- MOD_DEC_USE_COUNT;
return err;
}
@@ -171,7 +169,6 @@
if (ap->tpkt != 0)
kfree_skb(ap->tpkt);
kfree(ap);
- MOD_DEC_USE_COUNT;
}
/*
@@ -310,6 +307,9 @@
receive_room: ppp_asynctty_room,
receive_buf: ppp_asynctty_receive,
write_wakeup: ppp_asynctty_wakeup,
+#ifdef CONFIG_MODULES
+ owner: THIS_MODULE,
+#endif
};
static int __init
--- linux-2.4.2-ac24/drivers/char/n_hdlc.c Sun Feb 25 17:37:03 2001
+++ ac/drivers/char/n_hdlc.c Sun Mar 25 23:12:03 2001
@@ -305,7 +305,6 @@
n_hdlc->tty = n_hdlc->backup_tty;
} else {
n_hdlc_release (n_hdlc);
- MOD_DEC_USE_COUNT;
}
}
@@ -345,8 +344,6 @@
tty->disc_data = n_hdlc;
n_hdlc->tty = tty;
- MOD_INC_USE_COUNT;
-
#if defined(TTY_NO_WRITE_SPLIT)
/* change tty_io write() to not split large writes into 8K chunks */
set_bit(TTY_NO_WRITE_SPLIT,&tty->flags);
@@ -1006,6 +1003,9 @@
n_hdlc_ldisc.receive_room = n_hdlc_tty_room;
n_hdlc_ldisc.receive_buf = n_hdlc_tty_receive;
n_hdlc_ldisc.write_wakeup = n_hdlc_tty_wakeup;
+#ifdef CONFIG_MODULES
+ n_hdlc_ldisc.owner = THIS_MODULE;
+#endif
status = tty_register_ldisc(N_HDLC, &n_hdlc_ldisc);
if (!status)
--- linux-2.4.2-ac24/drivers/char/n_r3964.c Sat Mar 24 14:28:04 2001
+++ ac/drivers/char/n_r3964.c Sun Mar 25 23:15:51 2001
@@ -146,22 +146,20 @@
static int r3964_receive_room(struct tty_struct *tty);
static struct tty_ldisc tty_ldisc_N_R3964 = {
- TTY_LDISC_MAGIC, /* magic */
- "R3964", /* name */
- 0, /* num */
- 0, /* flags */
- r3964_open, /* open */
- r3964_close, /* close */
- 0, /* flush_buffer */
- 0, /* chars_in_buffer */
- r3964_read, /* read */
- r3964_write, /* write */
- r3964_ioctl, /* ioctl */
- r3964_set_termios, /* set_termios */
- r3964_poll, /* poll */
- r3964_receive_buf, /* receive_buf */
- r3964_receive_room, /* receive_room */
- 0 /* write_wakeup */
+ magic: TTY_LDISC_MAGIC,
+ name: "R3964",
+ open: r3964_open,
+ close: r3964_close,
+ read: r3964_read,
+ write: r3964_write,
+ ioctl: r3964_ioctl,
+ set_termios: r3964_set_termios,
+ poll: r3964_poll,
+ receive_buf: r3964_receive_buf,
+ receive_room: r3964_receive_room,
+#ifdef CONFIG_MODULES
+ owner: THIS_MODULE,
+#endif
};
@@ -1098,8 +1096,6 @@
{
struct r3964_info *pInfo;
- MOD_INC_USE_COUNT;
-
TRACE_L("open");
TRACE_L("tty=%x, PID=%d, disc_data=%x",
(int)tty, current->pid, (int)tty->disc_data);
@@ -1234,8 +1230,6 @@
TRACE_M("r3964_close - tx_buf kfree %x",(int)pInfo->tx_buf);
kfree(pInfo);
TRACE_M("r3964_close - info kfree %x",(int)pInfo);
-
- MOD_DEC_USE_COUNT;
}
static int r3964_read(struct tty_struct *tty, struct file *file,
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
