From: David van Moolenbroek Date: Tue, 26 Aug 2014 22:28:58 +0000 (+0000) Subject: VFS: unbreak select on /dev/tty X-Git-Tag: v3.3.0~44 X-Git-Url: http://zhaoyanbai.com/repos/%22http:/www.isc.org/icons/zlib_tech.html?a=commitdiff_plain;h=27d0ecdb624228487a475d72d67d3320abb40fc8;p=minix.git VFS: unbreak select on /dev/tty The remapping from /dev/tty to the real controlling terminal in the device code was confusing the select code. The latter is now aware of this case and should handle it properly, at the cost of one extra field in the filp structure. There is a nasty, hopefully sufficiently rare case of /dev/tty being kept open while controlling terminals are changing, that we are still not handling. Doing so would require more than just a few changes, but the code should at least detect and cleanly fail on this case. Test77 now has a basic test set for selecting on /dev/tty. Change-Id: Iaedea449cdb728d0e66a9de8faacdfd9638dfe92 --- diff --git a/minix/servers/vfs/device.c b/minix/servers/vfs/device.c index c76951471..7d34c0479 100644 --- a/minix/servers/vfs/device.c +++ b/minix/servers/vfs/device.c @@ -200,39 +200,57 @@ static cp_grant_id_t make_grant(endpoint_t driver_e, endpoint_t user_e, int op, } /*===========================================================================* - * cdev_get * + * cdev_map * *===========================================================================*/ -static struct dmap *cdev_get(dev_t dev, devminor_t *minor_dev) +dev_t cdev_map(dev_t dev, struct fproc *rfp) { -/* Obtain the dmap structure for the given device, if a valid driver exists for - * the major device. Perform redirection for CTTY_MAJOR. +/* Map the given device number to a real device number, remapping /dev/tty to + * the given process's controlling terminal if it has one. Perform a bounds + * check on the resulting device's major number, and return NO_DEV on failure. + * This function is idempotent but not used that way. */ - devmajor_t major_dev; - struct dmap *dp; - int slot; + devmajor_t major; /* First cover one special case: /dev/tty, the magic device that translates * to the controlling tty. */ - if (major(dev) == CTTY_MAJOR) { + if ((major = major(dev)) == CTTY_MAJOR) { /* No controlling terminal? Fail the request. */ - if (fp->fp_tty == 0) return(NULL); + if (rfp->fp_tty == NO_DEV) return NO_DEV; /* Substitute the controlling terminal device. */ - dev = fp->fp_tty; + dev = rfp->fp_tty; + major = major(dev); } - /* Determine task dmap. */ - major_dev = major(dev); - if (major_dev < 0 || major_dev >= NR_DEVICES) return(NULL); + if (major < 0 || major >= NR_DEVICES) return NO_DEV; - dp = &dmap[major_dev]; + return dev; +} + +/*===========================================================================* + * cdev_get * + *===========================================================================*/ +static struct dmap *cdev_get(dev_t dev, devminor_t *minor_dev) +{ +/* Obtain the dmap structure for the given device, if a valid driver exists for + * the major device. Perform redirection for CTTY_MAJOR. + */ + struct dmap *dp; + int slot; + + /* Remap /dev/tty as needed. Perform a bounds check on the major number. */ + if ((dev = cdev_map(dev, fp)) == NO_DEV) + return(NULL); + + /* Determine task dmap. */ + dp = &dmap[major(dev)]; /* See if driver is roughly valid. */ if (dp->dmap_driver == NONE) return(NULL); if (isokendpt(dp->dmap_driver, &slot) != OK) { - printf("VFS: cdev_get: old driver for major %x (%d)\n", major_dev, + printf("VFS: cdev_get: old driver for major %x (%d)\n", major(dev), dp->dmap_driver); return(NULL); } @@ -531,21 +549,27 @@ int do_ioctl(void) *===========================================================================*/ int cdev_select(dev_t dev, int ops) { -/* Initiate a select call on a device. Return OK iff the request was sent. */ - devminor_t minor_dev; +/* Initiate a select call on a device. Return OK iff the request was sent. + * This function explicitly bypasses cdev_get() since it must not do CTTY + * mapping, because a) the caller already has done that, b) "fp" may be wrong. + */ + devmajor_t major; message dev_mess; struct dmap *dp; int r; - /* Determine task dmap. */ - if ((dp = cdev_get(dev, &minor_dev)) == NULL) - return(EIO); + /* Determine task dmap, without CTTY mapping. */ + assert(dev != NO_DEV); + major = major(dev); + assert(major >= 0 && major < NR_DEVICES); + assert(major != CTTY_MAJOR); + dp = &dmap[major]; /* Prepare the request message. */ memset(&dev_mess, 0, sizeof(dev_mess)); dev_mess.m_type = CDEV_SELECT; - dev_mess.m_vfs_lchardriver_select.minor = minor_dev; + dev_mess.m_vfs_lchardriver_select.minor = minor(dev); dev_mess.m_vfs_lchardriver_select.ops = ops; /* Send the request to the driver. */ diff --git a/minix/servers/vfs/file.h b/minix/servers/vfs/file.h index efbe80d97..a1a4a51e0 100644 --- a/minix/servers/vfs/file.h +++ b/minix/servers/vfs/file.h @@ -29,6 +29,7 @@ EXTERN struct filp { /* following are for fd-type-specific select() */ int filp_pipe_select_ops; + dev_t filp_char_select_dev; } filp[NR_FILPS]; #define FILP_CLOSED 0 /* filp_mode: associated device closed/gone */ diff --git a/minix/servers/vfs/filedes.c b/minix/servers/vfs/filedes.c index a810feb2f..6adc70386 100644 --- a/minix/servers/vfs/filedes.c +++ b/minix/servers/vfs/filedes.c @@ -119,6 +119,7 @@ int get_fd(struct fproc *rfp, int start, mode_t bits, int *k, struct filp **fpt) f->filp_selectors = 0; f->filp_select_ops = 0; f->filp_pipe_select_ops = 0; + f->filp_char_select_dev = NO_DEV; f->filp_flags = 0; f->filp_select_flags = 0; f->filp_softlock = NULL; diff --git a/minix/servers/vfs/misc.c b/minix/servers/vfs/misc.c index 65617d2a3..65d4b8630 100644 --- a/minix/servers/vfs/misc.c +++ b/minix/servers/vfs/misc.c @@ -668,7 +668,7 @@ static void free_proc(int flags) if (vp->v_sdev != dev) continue; lock_filp(rfilp, VNODE_READ); (void) cdev_close(dev); /* Ignore any errors. */ - + /* FIXME: missing select check */ rfilp->filp_mode = FILP_CLOSED; unlock_filp(rfilp); } diff --git a/minix/servers/vfs/proto.h b/minix/servers/vfs/proto.h index ad0449dc9..051a807b6 100644 --- a/minix/servers/vfs/proto.h +++ b/minix/servers/vfs/proto.h @@ -34,6 +34,7 @@ int cdev_open(dev_t dev, int flags); int cdev_close(dev_t dev); int cdev_io(int op, dev_t dev, endpoint_t proc_e, vir_bytes buf, off_t pos, unsigned long bytes, int flags); +dev_t cdev_map(dev_t dev, struct fproc *rfp); int cdev_select(dev_t dev, int ops); int cdev_cancel(dev_t dev); void cdev_reply(void); diff --git a/minix/servers/vfs/select.c b/minix/servers/vfs/select.c index 66ff4ffad..5bf2a1171 100644 --- a/minix/servers/vfs/select.c +++ b/minix/servers/vfs/select.c @@ -57,9 +57,12 @@ static int is_regular_file(struct filp *f); static int is_pipe(struct filp *f); static int is_char_device(struct filp *f); static void select_lock_filp(struct filp *f, int ops); -static int select_request_file(struct filp *f, int *ops, int block); -static int select_request_char(struct filp *f, int *ops, int block); -static int select_request_pipe(struct filp *f, int *ops, int block); +static int select_request_file(struct filp *f, int *ops, int block, + struct fproc *rfp); +static int select_request_char(struct filp *f, int *ops, int block, + struct fproc *rfp); +static int select_request_pipe(struct filp *f, int *ops, int block, + struct fproc *rfp); static void select_cancel_all(struct selectentry *e); static void select_cancel_filp(struct filp *f); static void select_return(struct selectentry *); @@ -68,7 +71,8 @@ static int tab2ops(int fd, struct selectentry *e); static void wipe_select(struct selectentry *s); static struct fdtype { - int (*select_request)(struct filp *, int *ops, int block); + int (*select_request)(struct filp *, int *ops, int block, + struct fproc *rfp); int (*type_match)(struct filp *f); } fdtypes[] = { { select_request_char, is_char_device }, @@ -237,7 +241,7 @@ int do_select(void) wantops = (f->filp_select_ops |= ops); type = se->type[fd]; select_lock_filp(f, wantops); - r = fdtypes[type].select_request(f, &wantops, se->block); + r = fdtypes[type].select_request(f, &wantops, se->block, fp); unlock_filp(f); if (r != OK && r != SUSPEND) { se->error = r; @@ -355,19 +359,48 @@ static int is_char_device(struct filp *f) /*===========================================================================* * select_request_char * *===========================================================================*/ -static int select_request_char(struct filp *f, int *ops, int block) +static int select_request_char(struct filp *f, int *ops, int block, + struct fproc *rfp) { /* Check readiness status on a character device. Unless suitable results are * available right now, this will only initiate the polling process, causing * result processing to be deferred. This function MUST NOT block its calling * thread. The given filp may or may not be locked. */ - devmajor_t major; + dev_t dev; int r, rops; struct dmap *dp; - major = major(f->filp_vno->v_sdev); - if (major < 0 || major >= NR_DEVICES) return(ENXIO); + /* Start by remapping the device node number to a "real" device number. Those + * two are different only for CTTY_MAJOR aka /dev/tty, but that one single + * exception requires quite some extra effort here: the select code matches + * character driver replies to their requests based on the device number, so + * it needs to be aware that device numbers may be mapped. The idea is to + * perform the mapping once and store the result in the filp object, so that + * at least we don't run into problems when a process loses its controlling + * terminal while doing a select (see also free_proc). It should be noted + * that it is possible that multiple processes share the same /dev/tty filp, + * and they may not all have a controlling terminal. The ctty-less processes + * should never pass the mapping; a more problematic case is checked below. + * + * The cdev_map call also checks the major number for rough validity, so that + * we can use it to index the dmap array safely a bit later. + */ + if ((dev = cdev_map(f->filp_vno->v_sdev, rfp)) == NO_DEV) + return(ENXIO); + + if (f->filp_char_select_dev != NO_DEV && f->filp_char_select_dev != dev) { + /* Currently, this case can occur as follows: a process with a + * controlling terminal opens /dev/tty and forks, the new child starts + * a new session, opens a new controlling terminal, and both parent and + * child call select on the /dev/tty file descriptor. If this case ever + * becomes real, a better solution may be to force-close a filp for + * /dev/tty when a new controlling terminal is opened. + */ + printf("VFS: file pointer has multiple controlling TTYs!\n"); + return(EIO); + } + f->filp_char_select_dev = dev; /* set before possibly suspending */ rops = *ops; @@ -401,12 +434,12 @@ static int select_request_char(struct filp *f, int *ops, int block) if (f->filp_select_flags & FSF_BUSY) return(SUSPEND); - dp = &dmap[major]; + dp = &dmap[major(dev)]; if (dp->dmap_sel_busy) return(SUSPEND); f->filp_select_flags &= ~FSF_UPDATE; - r = cdev_select(f->filp_vno->v_sdev, rops); + r = cdev_select(dev, rops); if (r != OK) return(r); @@ -421,7 +454,7 @@ static int select_request_char(struct filp *f, int *ops, int block) * select_request_file * *===========================================================================*/ static int select_request_file(struct filp *UNUSED(f), int *UNUSED(ops), - int UNUSED(block)) + int UNUSED(block), struct fproc *UNUSED(rfp)) { /* Files are always ready, so output *ops is input *ops */ return(OK); @@ -430,7 +463,8 @@ static int select_request_file(struct filp *UNUSED(f), int *UNUSED(ops), /*===========================================================================* * select_request_pipe * *===========================================================================*/ -static int select_request_pipe(struct filp *f, int *ops, int block) +static int select_request_pipe(struct filp *f, int *ops, int block, + struct fproc *UNUSED(rfp)) { /* Check readiness status on a pipe. The given filp is locked. This function * may block its calling thread if necessary. @@ -612,13 +646,15 @@ static void select_cancel_filp(struct filp *f) /* If this filp is the subject of an ongoing select query to a * character device, mark the query as stale, so that this filp will - * not be checked when the result arrives. + * not be checked when the result arrives. The filp select device may + * still be NO_DEV if do_select fails on the initial fd check. */ - if (is_char_device(f)) { - major = major(f->filp_vno->v_sdev); + if (is_char_device(f) && f->filp_char_select_dev != NO_DEV) { + major = major(f->filp_char_select_dev); if (dmap[major].dmap_sel_busy && dmap[major].dmap_sel_filp == f) dmap[major].dmap_sel_filp = NULL; /* leave _busy set */ + f->filp_char_select_dev = NO_DEV; } } } @@ -746,10 +782,11 @@ void select_unsuspend_by_endpt(endpoint_t proc_e) } for (fd = 0; fd < se->nfds; fd++) { - if ((f = se->filps[fd]) == NULL || f->filp_vno == NULL) + if ((f = se->filps[fd]) == NULL || !is_char_device(f)) continue; - major = major(f->filp_vno->v_sdev); + assert(f->filp_char_select_dev != NO_DEV); + major = major(f->filp_char_select_dev); if (dmap_driver_match(proc_e, major)) { se->filps[fd] = NULL; se->error = EIO; @@ -775,7 +812,6 @@ void select_reply1(endpoint_t driver_e, devminor_t minor, int status) dev_t dev; struct filp *f; struct dmap *dp; - struct vnode *vp; /* Figure out which device is replying */ if ((dp = get_dmap(driver_e)) == NULL) return; @@ -796,15 +832,14 @@ void select_reply1(endpoint_t driver_e, devminor_t minor, int status) */ if ((f = dp->dmap_sel_filp) != NULL) { /* Find vnode and check we got a reply from the device we expected */ - vp = f->filp_vno; - assert(vp != NULL); - assert(S_ISCHR(vp->v_mode)); - if (vp->v_sdev != dev) { + assert(is_char_device(f)); + assert(f->filp_char_select_dev != NO_DEV); + if (f->filp_char_select_dev != dev) { /* This should never happen. The driver may be misbehaving. * For now we assume that the reply we want will arrive later.. */ printf("VFS (%s:%d): expected reply from dev %llx not %llx\n", - __FILE__, __LINE__, vp->v_sdev, dev); + __FILE__, __LINE__, f->filp_char_select_dev, dev); return; } } @@ -872,7 +907,6 @@ void select_reply2(endpoint_t driver_e, devminor_t minor, int status) dev_t dev; struct filp *f; struct dmap *dp; - struct vnode *vp; struct selectentry *se; if (status == 0) { @@ -898,9 +932,9 @@ void select_reply2(endpoint_t driver_e, devminor_t minor, int status) found = FALSE; for (fd = 0; fd < se->nfds; fd++) { if ((f = se->filps[fd]) == NULL) continue; - if ((vp = f->filp_vno) == NULL) continue; - if (!S_ISCHR(vp->v_mode)) continue; - if (vp->v_sdev != dev) continue; + if (!is_char_device(f)) continue; + assert(f->filp_char_select_dev != NO_DEV); + if (f->filp_char_select_dev != dev) continue; if (status > 0) { /* Operations ready */ /* Clear the replied bits from the request @@ -974,7 +1008,7 @@ static void select_restart_filps(void) assert(is_char_device(f)); wantops = ops = f->filp_select_ops; - r = select_request_char(f, &wantops, se->block); + r = select_request_char(f, &wantops, se->block, se->requestor); if (r != OK && r != SUSPEND) { se->error = r; restart_proc(se); diff --git a/minix/tests/test77.c b/minix/tests/test77.c index fe0afcbc0..e1e6c7290 100644 --- a/minix/tests/test77.c +++ b/minix/tests/test77.c @@ -482,6 +482,157 @@ test77e(void) if (sigaction(SIGUSR1, &usr_oact, NULL) < 0) e(16); } +/* + * Test basic select functionality on /dev/tty. While this test should not be + * part of this test set, we already have all the infrastructure we need here. + */ +static void +test77f(void) +{ + struct sigaction act, oact; + char c, pname[PATH_MAX], tname[PATH_MAX]; + struct timeval tv; + fd_set fd_set; + int fd, maxfd, masterfd, slavefd; + + subtest = 6; + + /* We do not want to get SIGHUP signals in this test. */ + memset(&act, 0, sizeof(act)); + act.sa_handler = SIG_IGN; + if (sigaction(SIGHUP, &act, &oact) < 0) e(1); + + /* Get master and slave device names for a free pseudo terminal. */ + get_names(pname, tname); + + if ((masterfd = open(pname, O_RDWR | O_NOCTTY)) < 0) e(2); + + switch (fork()) { + case 0: + if (setsid() < 0) e(3); + + close(masterfd); + + if ((slavefd = open(tname, O_RDWR)) < 0) e(4); + + if ((fd = open("/dev/tty", O_RDWR)) < 0) e(5); + + make_raw(fd); + + /* Without slave input, /dev/tty is not ready for reading. */ + FD_ZERO(&fd_set); + FD_SET(fd, &fd_set); + tv.tv_sec = 0; + tv.tv_usec = 0; + + if (select(fd + 1, &fd_set, NULL, NULL, &tv) != 0) e(6); + if (FD_ISSET(fd, &fd_set)) e(7); + + FD_SET(fd, &fd_set); + tv.tv_sec = 0; + tv.tv_usec = 10000; + + if (select(fd + 1, &fd_set, NULL, NULL, &tv) != 0) e(8); + if (FD_ISSET(fd, &fd_set)) e(9); + + /* It will be ready for writing, though. */ + FD_SET(fd, &fd_set); + + if (select(fd + 1, NULL, &fd_set, NULL, NULL) != 1) e(10); + if (!FD_ISSET(fd, &fd_set)) e(11); + + /* Test mixing file descriptors to the same terminal. */ + FD_ZERO(&fd_set); + FD_SET(fd, &fd_set); + FD_SET(slavefd, &fd_set); + tv.tv_sec = 0; + tv.tv_usec = 10000; + + maxfd = fd > slavefd ? fd : slavefd; + if (select(maxfd + 1, &fd_set, NULL, NULL, &tv) != 0) e(12); + if (FD_ISSET(fd, &fd_set)) e(13); + if (FD_ISSET(slavefd, &fd_set)) e(14); + + /* The delayed echo on the master must wake up our select. */ + c = 'A'; + if (write(slavefd, &c, sizeof(c)) != sizeof(c)) e(15); + + FD_ZERO(&fd_set); + FD_SET(fd, &fd_set); + + if (select(fd + 1, &fd_set, NULL, NULL, NULL) != 1) e(16); + if (!FD_ISSET(fd, &fd_set)) e(17); + + /* Select must now still flag readiness for reading. */ + tv.tv_sec = 0; + tv.tv_usec = 0; + + if (select(fd + 1, &fd_set, NULL, NULL, &tv) != 1) e(18); + if (!FD_ISSET(fd, &fd_set)) e(19); + + /* That is, until we read the byte. */ + if (read(slavefd, &c, sizeof(c)) != sizeof(c)) e(20); + if (c != 'B') e(21); + + if (select(fd + 1, &fd_set, NULL, NULL, &tv) != 0) e(22); + if (FD_ISSET(fd, &fd_set)) e(23); + + /* Ask the parent to close the master. */ + c = 'C'; + if (write(slavefd, &c, sizeof(c)) != sizeof(c)) e(24); + + FD_SET(fd, &fd_set); + + /* The closure must cause an EOF condition on the slave. */ + if (select(fd + 1, &fd_set, NULL, NULL, NULL) != 1) e(25); + if (!FD_ISSET(fd, &fd_set)) e(26); + + if (select(fd + 1, &fd_set, NULL, NULL, NULL) != 1) e(27); + if (!FD_ISSET(fd, &fd_set)) e(28); + + if (read(slavefd, &c, sizeof(c)) != 0) e(29); + + exit(errct); + case -1: + e(30); + default: + /* Wait for the child to write something to the slave. */ + FD_ZERO(&fd_set); + FD_SET(masterfd, &fd_set); + + if (select(masterfd + 1, &fd_set, NULL, NULL, NULL) != 1) + e(31); + if (!FD_ISSET(masterfd, &fd_set)) e(32); + + if (read(masterfd, &c, sizeof(c)) != sizeof(c)) e(33); + if (c != 'A') e(34); + + /* Write a reply once the child is blocked in its select. */ + tv.tv_sec = 1; + tv.tv_usec = 0; + if (select(masterfd + 1, &fd_set, NULL, NULL, &tv) != 0) + e(35); + + c = 'B'; + if (write(masterfd, &c, sizeof(c)) != sizeof(c)) e(36); + + /* Wait for the child to request closing the master. */ + if (read(masterfd, &c, sizeof(c)) != sizeof(c)) e(37); + if (c != 'C') e(38); + + /* Close the master once the child is blocked in its select. */ + sleep(1); + + close(masterfd); + + break; + } + + if (waitchild() < 0) e(39); + + if (sigaction(SIGHUP, &oact, NULL) < 0) e(28); +} + int main(int argc, char **argv) { @@ -500,6 +651,7 @@ main(int argc, char **argv) if (m & 0x04) test77c(); if (m & 0x08) test77d(); if (m & 0x10) test77e(); + if (m & 0x20) test77f(); } quit();