Device driver attachment when using module(7)
Hi all, I have one question about how device drivers are supposed to be configured when loaded as modules. I've noticed this is the most commonly used method of attachment throughout the kernel: ... switch (cmd) { case MODULE_CMD_INIT: #ifdef _MODULE error = config_init_component(cfdriver_ioconf_if_ath_pci, cfattach_ioconf_if_ath_pci, cfdata_ioconf_if_ath_pci); #endif return error; case MODULE_CMD_FINI: ... And so on. Since I rarely used modules when looking for particular drivers, I just noticed that by loading (and unloading) a module from the command line, no new device is recognized or attached. In some cases, especially in pci drivers, just a rescan message is shown (both in console and dmesg). So my question is: are module device drivers supposed to configure a real device driver instance, or are they just a method of inserting the right autoconf glue, before the whole autoconfiguration process take place? Shouldn't a device driver module also perform some kind of bus enumeration scan, and then attach a new real device instance (by calling config_attach, or config_found, etc..), especially if requested from the command line? Thanks in advance T.G.
RE: In-kernel process exit hooks?
On Wed, 6 Jan 2016, Terry Moore wrote: You may well be right. From looking at the man page, fd_getfile() assumes the the file is already open. Is there an additional spec_write() after the fd_getfile()? I don't see it in you patch. spec_write() is called via the dereferencing at the end of filemon_output() ... In any case, I was using writabality as an example. It's really fragile to depend on grabbing a file handle and assuming it's what you had before. The security analysis of that is essentially open-ended -- and has to be revisited every time the behavior of files as seen by fd_getfile() changes [therefore is an eternal burden], whereas the analysis of adding an additional exit hook is trivial, and as far as I can see, never has to be revisited. Point taken. In this case, write access is checked on each call, so that's not a problem. But without holding the fd_getfile() reference, the application program is indeed frre to switch things out from under us. I've attempted to minimize that by comparing the pointers to the 'struct file' but it doesn't guarantee that things have not changed. One more reason for us to retain the extra reference as originally written, and then modify exithooks as previously proposed to clean things up in the correct order. I understand everyone wanting to be conservative about not adding new facilities to the kernel, but sometimes a new facility actually saves a lot of effort overall. You're doing the work, so having made my point, I'll let you make your decision. --Terry -Original Message- From: tech-kern-ow...@netbsd.org [mailto:tech-kern-ow...@netbsd.org] On Behalf Of Paul Goyette Sent: Wednesday, January 6, 2016 21:31 To: Terry Moore Cc: tech-kern@netbsd.org Subject: RE: In-kernel process exit hooks? I'm pretty sure that the mode check done at the beginning of spec_write() will ensure that the file is opened with write access. :) On Wed, 6 Jan 2016, Terry Moore wrote: Isn't there a security risk with the fd_getfile() approach? This sounds (on the face of it) similar to the kinds of problems that led tmpnam(3) to be deprecated? For example, what if the monitoring program deliberately points the fd at a file that it opened as read-only; will filemon then write to it? --Terry -Original Message- From: tech-kern-ow...@netbsd.org [mailto:tech-kern-ow...@netbsd.org] On Behalf Of Paul Goyette Sent: Wednesday, January 6, 2016 16:55 To: Taylor R Campbell Cc: tech-kern@netbsd.org Subject: Re: In-kernel process exit hooks? Another possibility would be to change filemon(4) to do fd_getfile each it needs to use the file descriptor. This makes it a little more brittle (fails if you close the descriptor), but would sidestep the problem. Hmmm, perhaps. Failure would not be a problem, since we would just revert to the initial "output file unspecified" conditions. I think I like this approach. :) I'll give it a try. This actually works quite well. Please see the attached diffs for your review. One possible problem is what happens if the monitoring program closes the file descriptor, and then re-uses that fd? I've included a check to compare the original 'struct file *' pointer with the current one, which will catch "some" instances, but not guaranteed to catch them all. It could be a bit of a surprise if filemon output shows up in unexpected places. :) Because of this potential for surprising the user, I think I'm still leaning to my earlier proposal of extending exithook processing. But given the limited number of use-cases for filemon, I could live with making the fd_getfile()-only-when-you-need-it change instead. +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at | netbsd.org | +--+--++ +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++ +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
RE: In-kernel process exit hooks?
I'm pretty sure that the mode check done at the beginning of spec_write() will ensure that the file is opened with write access. :) On Wed, 6 Jan 2016, Terry Moore wrote: Isn't there a security risk with the fd_getfile() approach? This sounds (on the face of it) similar to the kinds of problems that led tmpnam(3) to be deprecated? For example, what if the monitoring program deliberately points the fd at a file that it opened as read-only; will filemon then write to it? --Terry -Original Message- From: tech-kern-ow...@netbsd.org [mailto:tech-kern-ow...@netbsd.org] On Behalf Of Paul Goyette Sent: Wednesday, January 6, 2016 16:55 To: Taylor R Campbell Cc: tech-kern@netbsd.org Subject: Re: In-kernel process exit hooks? Another possibility would be to change filemon(4) to do fd_getfile each it needs to use the file descriptor. This makes it a little more brittle (fails if you close the descriptor), but would sidestep the problem. Hmmm, perhaps. Failure would not be a problem, since we would just revert to the initial "output file unspecified" conditions. I think I like this approach. :) I'll give it a try. This actually works quite well. Please see the attached diffs for your review. One possible problem is what happens if the monitoring program closes the file descriptor, and then re-uses that fd? I've included a check to compare the original 'struct file *' pointer with the current one, which will catch "some" instances, but not guaranteed to catch them all. It could be a bit of a surprise if filemon output shows up in unexpected places. :) Because of this potential for surprising the user, I think I'm still leaning to my earlier proposal of extending exithook processing. But given the limited number of use-cases for filemon, I could live with making the fd_getfile()-only-when-you-need-it change instead. +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++ +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: cacheline-aligned locks [was Re: RFC: gif(4) MP-ify]
Hi, On 2016/01/07 0:26, Taylor R Campbell wrote: >Date: Wed, 6 Jan 2016 14:19:01 +0900 >From: Kengo NAKAHARA > >I have a question for confirmation. If the struct has two locks for >different purposes, is the COHERENCY_UNIT padding required between >the locks isn't it? E.g. in my old patch(gif-mp-ify.patch), is the >COHERENCY_UNIT padding is required between gif_lock and struct si_sync >which has other lock(si_lock)? ># BTW, struct si_sync is removed by if_gif.h:r1.21 > > That may be a good idea if you expect simultaneous use of both locks > independently -- as in struct pcq. But without that analysis, it is > likely premature optimization, and the wrangling of COHERENCY_UNIT > alignment inside a struct is more trouble than it's worth. In > contrast, for global locks, the __cacheline_aligned attribute is no > trouble at all. I see. I will analyze carefully if I want to use COHERENCY_UNIT inside struct. Thank you very much! Thanks, -- // Internet Initiative Japan Inc. Device Engineering Section, Core Product Development Department, Product Division, Technology Unit Kengo NAKAHARA
Re: In-kernel process exit hooks?
> Another possibility would be to change filemon(4) to do fd_getfile > each it needs to use the file descriptor. This makes it a little > more brittle (fails if you close the descriptor), but would sidestep > the problem. Hmmm, perhaps. Failure would not be a problem, since we would just revert to the initial "output file unspecified" conditions. I think I like this approach. :) I'll give it a try. This actually works quite well. Please see the attached diffs for your review. One possible problem is what happens if the monitoring program closes the file descriptor, and then re-uses that fd? I've included a check to compare the original 'struct file *' pointer with the current one, which will catch "some" instances, but not guaranteed to catch them all. It could be a bit of a surprise if filemon output shows up in unexpected places. :) Because of this potential for surprising the user, I think I'm still leaning to my earlier proposal of extending exithook processing. But given the limited number of use-cases for filemon, I could live with making the fd_getfile()-only-when-you-need-it change instead. +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++Index: filemon.c === RCS file: /cvsroot/src/sys/dev/filemon/filemon.c,v retrieving revision 1.24 diff -u -p -r1.24 filemon.c --- filemon.c 5 Jan 2016 22:08:54 - 1.24 +++ filemon.c 7 Jan 2016 00:45:46 - @@ -94,9 +94,21 @@ filemon_output(struct filemon * filemon, { struct uio auio; struct iovec aiov; + struct file *fp; /* Output file pointer. */ - if (filemon->fm_fp == NULL) + + if (filemon->fm_fp == NULL) /* No output file specified */ + return; + if ((fp = fd_getfile(filemon->fm_fd)) != filemon->fm_fp) { + /* +* The file descriptor refers to a different file +* than when it was passed to SET_FD. So assume we +* never had an output file. +*/ + filemon->fm_fp = NULL; + filemon->fm_fd = -1; return; + } aiov.iov_base = msg; aiov.iov_len = len; @@ -122,6 +134,7 @@ filemon_output(struct filemon * filemon, (*filemon->fm_fp->f_ops->fo_write) (filemon->fm_fp, &(filemon->fm_fp->f_offset), &auio, curlwp->l_cred, FOF_UPDATE_OFFSET); + fd_putfile(filemon->fm_fd); /* release our reference */ } void @@ -264,10 +277,7 @@ filemon_close(struct file * fp) * once we have exclusive access, it should never be used again */ rw_enter(&filemon->fm_mtx, RW_WRITER); - if (filemon->fm_fp) { - fd_putfile(filemon->fm_fd); /* release our reference */ - filemon->fm_fp = NULL; - } + filemon->fm_fp = NULL; rw_exit(&filemon->fm_mtx); rw_destroy(&filemon->fm_mtx); kmem_free(filemon, sizeof(struct filemon)); @@ -309,6 +319,7 @@ filemon_ioctl(struct file * fp, u_long c error = EBADF; break; } + fd_putfile(filemon->fm_fd); /* Write the file header. */ filemon_comment(filemon); break;
Re: In-kernel process exit hooks?
On Wed, 6 Jan 2016, Taylor R Campbell wrote: Another possibility would be to change filemon(4) to do fd_getfile each it needs to use the file descriptor. This makes it a little more brittle (fails if you close the descriptor), but would sidestep the problem. Hmmm, perhaps. Failure would not be a problem, since we would just revert to the initial "output file unspecified" conditions. I think I like this approach. :) I'll give it a try. Another possibility would be to use fd_getfile2/closef, which holds a reference only on the file, instead of fd_getfile/fd_putfile, which holds a reference on the file and on the descriptor. Releasing the reference to the file may not a problem, even though releasing the reference to the descriptor is a problem as you found. Would this prevent the file descriptor from being closed behind our backs? +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: In-kernel process exit hooks?
Another possibility would be to change filemon(4) to do fd_getfile each it needs to use the file descriptor. This makes it a little more brittle (fails if you close the descriptor), but would sidestep the problem. Another possibility would be to use fd_getfile2/closef, which holds a reference only on the file, instead of fd_getfile/fd_putfile, which holds a reference on the file and on the descriptor. Releasing the reference to the file may not a problem, even though releasing the reference to the descriptor is a problem as you found.
In-kernel process exit hooks?
Please see references [1], [2], and [3] for details on this issue ... Based on internal implementation of filemon(4), there is an ordering requirement imposed on the sequence of events that occur when a process using /dev/filemon exits. In particular, the file descriptor on which the device is open needs to be closed prior to closing the descriptor to which avent/activity records are being logged. (Otherwise, because of an extra reference on the log file, fd_close() will hang indefinitely.) It has been suggested to implement a filemon(4)-specific "exit hook" to ensure that the /dev/filemon device gets closed first. Instead of a hook that is specific to filemon(4), I would propose that a generic hook mechanism be added. This way, if any future ordering requirements are found, we won't need another feature-specific mechanism. We already have an exit_hook implementation, but unfortunately the hooks are calls too late in the exit processing. Particularly, the hooks are called after all files are closed (via fd_free()) and after the working directory of the process has been free()d (via cwdfree()). As far as I can tell, there are only two current users of the exithook mechanism (in kern/sys_aio.c and kern/sysv_sem.c). It is not clear to me if either of these users would be affected if the execution of the exithooks were to happen _before_ the call to fd_free(). I'd rather not have two sets of exithooks if it can be avoided. We could perhaps add a "phase" argument to the current exithook routines, using it to select between phase-specific LIST_HEADs. Similar to (in kern/kern_hook.c starting at line 222) static hook_list_t exithook_pre_close_list = LIST_HEAD_INITIALIZER(exithook_pre_close_list); static hook_list_t exithook_post_close_list = LIST_HEAD_INITIALIZER(exithook_post_close_list); static hook_list_t *exithook_table[] = { &exithook_pre_close_list, &exithook_post_close_list }; extern krwlock_t exec_lock; void * exithook_establish(void (*fn)(struct proc *, void *), void *arg, int ph) { void *rv; KASSERT(ph >= 0 && ph < __arraycount(exithook_table)); rw_enter(&exec_lock, RW_WRITER); rv = hook_establish(exithook_table[ph], (void (*)(void *))fn, arg); rw_exit(&exec_lock); return rv; } and with similar changes for exithook_disestablish() and doexithooks(). Then, in kern/kern_exit.c we simply have (at line 278+) ... /* * Close open files, release open-file table and free signal * actions. This may block! */ doexithooks(p, EXITHOOK_PRE_CLOSE); fd_free(); cwdfree(p->p_cwdi); p->p_cwdi = NULL; doexithooks(p, EXITHOOK_POST_CLOSE); sigactsfree(p->p_sigacts); ... Comments? Alternative approaches? [1] http://mail-index.netbsd.org/source-changes-d/2016/01/06/msg008307.html [2] http://mail-index.netbsd.org/tech-kern/2016/01/05/msg019896.html [3] http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=50627 +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: How to identify specific wait-state for a "DE" process?
On Wed, 6 Jan 2016, David Holland wrote: On Wed, Jan 06, 2016 at 08:10:36AM +0800, Paul Goyette wrote: > Does anyone have any good suggestions for how to arrange for another > thread/lwp to run so it can remove the extra reference to the logging > descriptor? A better suggestion: remove the broken behavior of close(). Hmm, perhaps. But that sounds like a much more intrusive, and much more risky, approach. :) +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: How to identify specific wait-state for a "DE" process?
On Wed, Jan 06, 2016 at 08:10:36AM +0800, Paul Goyette wrote: > Does anyone have any good suggestions for how to arrange for another > thread/lwp to run so it can remove the extra reference to the logging > descriptor? A better suggestion: remove the broken behavior of close(). -- David A. Holland dholl...@netbsd.org
Re: How to identify specific wait-state for a "DE" process?
Date: Wed, 6 Jan 2016 09:22:44 -0800 From: Brian Buhrow hello. Is there a particular reason file descriptors are closed in ascending order? Traditionally, file descriptors 2, 1 and 0 are always in use and it seems like it might be a good idea to have those be the last to get closed. I've seen some applications that close all their descriptors in descending order. I thought that was odd, but I think Paul just came up with a good reason to do such a thing. This only fixes the problem for certain orderings of file descriptors. I think the best way to fix this properly will be to just modify sys_exit to call a new filemon_prepare_exit routine that pre-closes any filemon-owned references to files, so that filemon_close itself will not hang.
Re: workqueue semantics [was Re: How to identify specific wait-state for a "DE" process?]
hello. Is there a particular reason file descriptors are closed in ascending order? Traditionally, file descriptors 2, 1 and 0 are always in use and it seems like it might be a good idea to have those be the last to get closed. I've seen some applications that close all their descriptors in descending order. I thought that was odd, but I think Paul just came up with a good reason to do such a thing. -Brian On Jan 6, 11:38am, Paul Goyette wrote: } Subject: Re: workqueue semantics [was Re: How to identify specific wait-st } On Wed, 6 Jan 2016, Taylor R Campbell wrote: } } > Date: Tue, 5 Jan 2016 21:48:42 -0500 } > From: Thor Lancelot Simon } > } > You can probably use workqueues for this. Looking at the manual page } > again for the first time in years, I think it's a little misleading -- } > what I believe is meant by "A work must not be enqueued again until the } > callback is called..." is really "a work item must not be re-enqueued } > before it has been processed by the *func callback", not the alternate, } > crazy reading that would imply workqueues can only have one enqueued } > item at a time. } > } > Your reading of the man page is correct: it is the struct work, not } > the struct workqueue *, that may not be reused until the callback is } > run. } > } > (I'm not sure how this would help for pgoyette's application, though.) } } I don't know how it would help, either. The best I can think of is to } have a periodic task run which checks to see if the file descriptor is } being closed; if yes, then the code could release the reference and } wake up the condvar waiter. But is this really a good thing to do? And } what would be an appropriate interval? } } } +--+--++ } | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | } | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | } | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | } +--+--++ >-- End of excerpt from Paul Goyette
cacheline-aligned locks [was Re: RFC: gif(4) MP-ify]
Date: Wed, 6 Jan 2016 14:19:01 +0900 From: Kengo NAKAHARA I have a question for confirmation. If the struct has two locks for different purposes, is the COHERENCY_UNIT padding required between the locks isn't it? E.g. in my old patch(gif-mp-ify.patch), is the COHERENCY_UNIT padding is required between gif_lock and struct si_sync which has other lock(si_lock)? # BTW, struct si_sync is removed by if_gif.h:r1.21 That may be a good idea if you expect simultaneous use of both locks independently -- as in struct pcq. But without that analysis, it is likely premature optimization, and the wrangling of COHERENCY_UNIT alignment inside a struct is more trouble than it's worth. In contrast, for global locks, the __cacheline_aligned attribute is no trouble at all.