cacheline-aligned locks [was Re: RFC: gif(4) MP-ify]

[Taylor R Campbell]

   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.

Re: How to identify specific wait-state for a "DE" process?

[Taylor R Campbell]

   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?]

[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.
-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

Re: In-kernel process exit hooks?

[Paul Goyette]

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 |
+--+--++

In-kernel process exit hooks?

[Paul Goyette]

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[] = {
_pre_close_list,
_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(_lock, RW_WRITER);
rv = hook_establish(exithook_table[ph], (void (*)(void *))fn, arg);
rw_exit(_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?

[Paul Goyette]

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: In-kernel process exit hooks?

[Paul Goyette]

> 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),
, 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(>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(>fm_mtx);
rw_destroy(>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?

[Paul Goyette]

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: cacheline-aligned locks [was Re: RFC: gif(4) MP-ify]

[Kengo NAKAHARA]

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?

[Paul Goyette]

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 |
+--+--++

Device driver attachment when using module(7)

[T. Glass]

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?

[Taylor R Campbell]

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.

Re: How to identify specific wait-state for a "DE" process?

[David Holland]

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