Re: In-kernel process exit hooks?

[Paul Goyette]

On Fri, 8 Jan 2016, Paul Goyette wrote:


The saga continues!   :)



Next, I'm going to have a look at fd_getfile2()/fclose() and see if that
is a viable approach.


Hmmm.  The comments in kern/kern_descrip.c for fd_getfile2() require 
that the process is not allowed to fork or exit "across this call" 
(which I assume means "until the reference to the struct file is 
released").


I'm not sure how to prevent this.  I could probably re-use the exithook 
mechanism from the previous approach to handle the exit case, but how to 
prevent fork() is another beast entirely (I think).  And to make things 
worse, the example code in the filemon(4) man page explicitly calls 
fork().


I'm quickly running out of ideas here...  I'm strongly leaning towards 
leaving the code alone, and more clearly documenting the conditions 
under which the hang occurs (ie, closure of the activity-log fd prior to 
disassociated the activity-log from the /dev/filemon fd, whether the 
close is done explicitly or as part of the sequential close that occurs 
at process exit).


Someone (Martin@, I think) earlier suggested modifying things to use 
cv_timedwait() in fd_close(), and modifying fd_free() to retry.  This 
might help in the process exit scenario, but doesn't address the case 
where the application process explicitly closes the file with the extra 
reference.


It might also be possible to modify fd_close() to have a 
filemon-specific close routine, similar to what is currently being done 
for knote_fdclose().  But this seems rather heavy-handed for something 
that has such a limited use-case as filemon(4).


The only other approach I can think of is to modify filemon(4) so it 
does not directly write any data to the activity log.  Rather than 
having the application provide a log fd (on which the extra reference 
needs to be taken), the application would read(2) from the filemon fd 
and handle the "logging" itself.  This would mean two additional trips 
across the kernel/userland boundary for every event, which feels like a 
rather costly approach.  It would also mean modifying make(1) (the only 
"production" use-case for filemon(4)).



Any other suggestions would be appreciated.


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

The saga continues!   :)

I previously posted one set of patches that changes filemon to use 
fd_{get,put}file() only when the activity-log file is actually being 
accessed.  This works, but doesn't prevent or detect when the 
application program closes-and-reopens that fd.


Attached to this Email is a separate patch, using exithooks.  The 
current exithook implementation is extended to have an additional 
"phase" argument, with values of EXITHOOK_BEFORE_CLOSE and 
EXITHOOK_AFTER_CLOSE.  (Existing uses of exithooks in sys_aio, sysv_sem, 
and rump are updated to use EXITHOOK_AFTER_CLOSE.)


Filemon(4) now establishes an EXITHOOK_BEFORE_CLOSE which searches the 
process's file descriptor table for entries to /dev/filemon.  If any are 
found, the associated activity-log file is disassociated from the 
filemon fd and the extra reference is released.


(Note that establishing the exithook is actually done through a config 
finalizer routine, since the built-in MODULE_CLASS_DRIVER modules are 
initialized before the exithook stuff in exec_init().)


This exithook approach also works.  There is still one problem, however. 
If the application program attempts to explicitly close the log-file fd 
while it is still associated with the filemon, we get the same hang.



Next, I'm going to have a look at fd_getfile2()/fclose() and see if that 
is a viable 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 |
+--+--++Index: dev/filemon/filemon.c
===
RCS file: /cvsroot/src/sys/dev/filemon/filemon.c,v
retrieving revision 1.24
diff -u -p -r1.24 filemon.c
--- dev/filemon/filemon.c   5 Jan 2016 22:08:54 -   1.24
+++ dev/filemon/filemon.c   7 Jan 2016 23:54:37 -
@@ -43,6 +43,8 @@ __KERNEL_RCSID(0, "$NetBSD: filemon.c,v 
 #include 
 #include 
 #include 
+#include 
+#include 
 
 #include "filemon.h"
 #include "ioconf.h"
@@ -89,6 +91,75 @@ static TAILQ_HEAD(, filemon) filemons_in
 static int logLevel = LOG_DEBUG;
 #endif
 
+/*
+ * Use an exithook to make sure that the filemon device is closed and
+ * our additional reference to the output file is released before the
+ * normal closure of open files.  The extra reference would otherwise
+ * cause process exit to hang indefinitely.
+ *
+ * We need to use a config_finalize() routine to register the hook
+ * due to system initialization sequence.
+ */
+void *hook = NULL; /* cookie from exithook_establish() */
+
+static int filemon_register_hook(device_t dev __unused);
+static voidfilemon_exithook(struct proc *p, void *arg);
+
+static int
+filemon_register_hook(device_t dev __unused)
+{
+
+   if (hook == NULL)
+   hook = exithook_establish(filemon_exithook, hook,
+   EXITHOOK_BEFORE_CLOSE);
+printf("%s: hook %p\n", __func__, hook);
+   if (hook != NULL)
+   return 0;
+   return ENXIO;
+}
+
+static void
+filemon_exithook(struct proc *p, void *arg)
+{
+   int fd;
+   struct filemon  *filemon;
+   fdfile_t*ff;
+   filedesc_t  *fdp;
+   fdtab_t *dt;
+
+   fdp = p->p_fd;
+   dt = fdp->fd_dt;
+   for (fd = 0; fd < dt->dt_nfiles; fd++) {
+   ff = dt->dt_ff[fd];
+   KASSERT(fd >= NDFDFILE ||
+   ff == (fdfile_t *)fdp->fd_dfdfile[fd]);
+   if (ff == NULL || ff->ff_file == NULL)
+   continue;
+   if (ff->ff_file->f_type != DTYPE_MISC ||
+   ff->ff_file->f_ops  != _fileops)
+   continue;
+printf("%s: found pid %d fd %d\n", __func__, p->p_pid, fd);
+   rw_enter(_mtx, RW_WRITER);
+   filemon = ff->ff_file->f_data;
+   if (filemon != NULL) {
+
+   /*
+* If we have an output file, release our
+* reference to it.
+*/
+   rw_enter(>fm_mtx, RW_WRITER);
+   if (filemon->fm_fp) {
+printf("%s: releasing %d\n", __func__, filemon->fm_fd);
+   KASSERT(p == curproc);
+   fd_putfile(filemon->fm_fd);
+   filemon->fm_fp = NULL;
+   }
+   rw_exit(>fm_mtx);
+   }
+   rw_exit(_mtx);
+   }
+}
+
 void
 filemon_output(struct filemon * filemon, char *msg, size_t len)
 {
@@ -216,6 +287,9 @@ filemon_open(dev_t dev, int oflags __unu
struct file *fp;
int error, fd;
 
+   if (hook == NULL)
+   return ENXIO;
+
/* falloc() will 

Re: Device driver attachment when using module(7)

[Michael van Elst]

thegl...@gmail.com ("T. Glass") writes:

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

Apparently that depends on the driver and gets more twists when you
consider standard UNIX devices (i.e. devsw), pseudo devices and cloning.

I don't think that there is already a common understanding how this
is "supposed" to work.

Having experienced the recent modularization of raidframe(4), I also doubt
that all the existing modular drivers actually work when loaded from the command
line (or via autoload, as is done for devsw and filesystems).

-- 
-- 
Michael van Elst
Internet: mlel...@serpens.de
"A potential Snark may lurk in every tree."

RE: In-kernel process exit hooks?

[Terry Moore]

Will the hang occur if make dies due to a signal (control-C, or kill -9, for
example)?

--Terry

> -Original Message-
> From: tech-kern-ow...@netbsd.org [mailto:tech-kern-ow...@netbsd.org] On
> Behalf Of Paul Goyette
> Sent: Thursday, January 7, 2016 17:00
> To: Taylor R Campbell 
> Cc: tech-kern@netbsd.org
> Subject: Re: In-kernel process exit hooks?
> 
> On Fri, 8 Jan 2016, Paul Goyette wrote:
> 
> > The saga continues!   :)
> >
> > 
> >
> > Next, I'm going to have a look at fd_getfile2()/fclose() and see if
> > that is a viable approach.
> 
> Hmmm.  The comments in kern/kern_descrip.c for fd_getfile2() require that
> the process is not allowed to fork or exit "across this call"
> (which I assume means "until the reference to the struct file is released").
> 
> I'm not sure how to prevent this.  I could probably re-use the exithook
> mechanism from the previous approach to handle the exit case, but how to
> prevent fork() is another beast entirely (I think).  And to make things
> worse, the example code in the filemon(4) man page explicitly calls fork().
> 
> I'm quickly running out of ideas here...  I'm strongly leaning towards
> leaving the code alone, and more clearly documenting the conditions under
> which the hang occurs (ie, closure of the activity-log fd prior to
> disassociated the activity-log from the /dev/filemon fd, whether the close
> is done explicitly or as part of the sequential close that occurs at process
> exit).
> 
> Someone (Martin@, I think) earlier suggested modifying things to use
> cv_timedwait() in fd_close(), and modifying fd_free() to retry.  This might
> help in the process exit scenario, but doesn't address the case where the
> application process explicitly closes the file with the extra reference.
> 
> It might also be possible to modify fd_close() to have a filemon-specific
> close routine, similar to what is currently being done for knote_fdclose().
> But this seems rather heavy-handed for something that has such a limited
> use-case as filemon(4).
> 
> The only other approach I can think of is to modify filemon(4) so it does
> not directly write any data to the activity log.  Rather than having the
> application provide a log fd (on which the extra reference needs to be
> taken), the application would read(2) from the filemon fd and handle the
> "logging" itself.  This would mean two additional trips across the
> kernel/userland boundary for every event, which feels like a rather costly
> approach.  It would also mean modifying make(1) (the only "production" use-
> case for filemon(4)).
> 
> 
> Any other suggestions would be appreciated.
> 
> 
> +--+--++
> | 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?

[Robert Elz]

Date:Fri, 8 Jan 2016 09:00:03 +0800 (PHT)
From:Paul Goyette 
Message-ID:  

  | Any other suggestions would be appreciated.

Another possibility would be to detach the logged-to file from the
process when logging is enabled (making the ioctl that attaches it
also be notionally a close from the process point of view).   But
keeping the reference in kernel.  When logging is disabled (for whatever
reason) do the kernel fd_close() at that point).

Do this, and the process can no longer do an explicit close to cause
a hang, and exit() won't close it in the normal sequence either, only
closing the log (or detaching it from the fd) would close that file.

That's also a change of filemon semantics, but I can't imagine that the
application (make) really wants to be accessing the file while filemon is
writing to it, does it?   And if it did, it could always just explicitly
open it again (though I haven't really thought through the effects of a dup()
at this point - perhaps forbid ioctl'ing to a file that has a ref
count > 1, so we know the fd is the only reference to it, once
the ioctl "closes" it, a dup can't happen, obviously.

Accessing the file via an independent open (before or after the ioctl)
should not have any of these problems, that's a different struct file.

kre

Re: In-kernel process exit hooks?

[Paul Goyette]

On Fri, 8 Jan 2016, Robert Elz wrote:


Another possibility would be to detach the logged-to file from the
process when logging is enabled (making the ioctl that attaches it
also be notionally a close from the process point of view).   But
keeping the reference in kernel.  When logging is disabled (for whatever
reason) do the kernel fd_close() at that point).


Is there a "supported" interface for detaching the file (or descriptor) 
from the process without closing it?



Do this, and the process can no longer do an explicit close to cause
a hang, and exit() won't close it in the normal sequence either, only
closing the log (or detaching it from the fd) would close that file.

That's also a change of filemon semantics, but I can't imagine that the
application (make) really wants to be accessing the file while filemon is
writing to it, does it?   And if it did, it could always just explicitly
open it again (though I haven't really thought through the effects of a dup()
at this point - perhaps forbid ioctl'ing to a file that has a ref
count > 1, so we know the fd is the only reference to it, once
the ioctl "closes" it, a dup can't happen, obviously.


I expect that make(1) assumes the log-fd is still available, and just 
follows the example code in filemon(4) man page: when finished logging, 
reset the file-pointer to 0 and start reading the data.



Accessing the file via an independent open (before or after the ioctl)
should not have any of these problems, that's a different struct file.


Yeah, I was trying to avoid the change in semantics.  :)  The fewer 
things I touch, the fewer things will go wrong, and I definitely don't 
want to break make, which would result in difficulties making[sic] a new 
version.  :)



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

OK, I now have a third way of handling the problem.

To recap the three options (refer to the attachments)

1. fd_getfile

   This option calls fd_getfile() each time it needs to access the
   activity log, and calls fd_putfile() after each use.  This way,
   the additional reference on the file descriptor lasts only for a
   short period of time, and does not exist at any time that it can
   be passed to fd_close().

   The biggest drawback here is that the user-land application can
   close() and re-open() this fd, possibly referring to a different
   file; this will not be visible to filemon, which will write new
   event records to any file that happens to be opened on this fd.

2. exithook

   This option extends the existing exithook() mechanism to have
   multiple "phases", one of which can happen before the process
   exit code calls fd_free() (which in turn calls fd_close() for all
   open file descriptors).  The exithook registered by filemon finds
   any usages of filemon and resets the activity-log, which releases
   the extra reference to the log fd.

   This option works well for normal process exit (including signal),
   but does not resolve the problem if the application itself calls
   close() on the log fd.  In that situation, the process will still
   hang.

   Additionally, setting this up correctly is awkward, due to the
   order in which kernel components are initialized.  (Modules of
   CLASS_DRIVER get loaded and initialized before exec_init() can
   set up the hook mechanisms, so we need to use a config_finalizer
   to establish the exit hook.)

3. filemon-fd_close

   This solution introduces a new, filemon-specific callback in
   fd_close() (but only if the filemon module is loaded or built-in).
   Each time a file descriptor is passed to fd_close, the callback
   is invoked.  The callback checks each usage of /dev/filemon and
   if that usage is logging activity to the file being closed, the
   activity-log is reset, releasing the extra reference.  Thus,
   after we return to fd_close() the reference count is normal and
   the file gets properly closed.

   The only drawback I see here is the additional overhead of the
   callback, on every call to fd_close().  The code catches every
   occurrence of the "hang" that I can find, and handles it cleanly.

I still need to think about the fd_getfile2()/fclose() approach to see 
if it meets our needs, but the comments that prohibit calling fork() 
would seem to preclude this mechanism.


The "detach file from userland" approach suggested by kre would also 
likely work, but I'm reluctant to change the semantics of filemon.






+--+--++
| 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 06:28:01 -
@@ -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;
}
+   

Re: In-kernel process exit hooks?

[bch]

On 1/7/16, Paul Goyette  wrote:
> On Fri, 8 Jan 2016, Paul Goyette wrote:
>
>> The saga continues!   :)
>>
>> 
>>
>> Next, I'm going to have a look at fd_getfile2()/fclose() and see if that
>> is a viable approach.
>
> Hmmm.  The comments in kern/kern_descrip.c for fd_getfile2() require
> that the process is not allowed to fork or exit "across this call"
> (which I assume means "until the reference to the struct file is
> released").
>
> I'm not sure how to prevent this.  I could probably re-use the exithook
> mechanism from the previous approach to handle the exit case, but how to
> prevent fork() is another beast entirely (I think).  And to make things
> worse, the example code in the filemon(4) man page explicitly calls
> fork().
>
> I'm quickly running out of ideas here...  I'm strongly leaning towards
> leaving the code alone, and more clearly documenting the conditions
> under which the hang occurs (ie, closure of the activity-log fd prior to
> disassociated the activity-log from the /dev/filemon fd, whether the
> close is done explicitly or as part of the sequential close that occurs
> at process exit).
>
> Someone (Martin@, I think) earlier suggested modifying things to use
> cv_timedwait() in fd_close(), and modifying fd_free() to retry.  This
> might help in the process exit scenario, but doesn't address the case
> where the application process explicitly closes the file with the extra
> reference.
>
> It might also be possible to modify fd_close() to have a
> filemon-specific close routine, similar to what is currently being done
> for knote_fdclose().  But this seems rather heavy-handed for something
> that has such a limited use-case as filemon(4).
>
> The only other approach I can think of is to modify filemon(4) so it
> does not directly write any data to the activity log.  Rather than
> having the application provide a log fd (on which the extra reference
> needs to be taken), the application would read(2) from the filemon fd
> and handle the "logging" itself.  This would mean two additional trips
> across the kernel/userland boundary for every event, which feels like a
> rather costly approach.  It would also mean modifying make(1) (the only
> "production" use-case for filemon(4)).

It adds a degree of complexity, but one could also have both
interfaces (i.e.: filemon(4) does the writing, or the app recv's the
data from filemon and does the writing.

> Any other suggestions would be appreciated.
>
>
> +--+--++
> | 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]

On Thu, 7 Jan 2016, Terry Moore wrote:


Will the hang occur if make dies due to a signal (control-C, or kill -9, for
example)?


It depends on which fd is numerically lower.  If the /dev/filemon fd is 
lower, it will get closed first, which removes the reference to the 
log-file fd.  If the log-file fd is numerically lower, it will get 
closed first, and will hang waiting for its reference count to clear.


In practice, I expect that make(1) will open /dev/filemon first, get an 
fd, and then open the log file and get another fd.  The fd's are 
assigned on a first-free basis, so /dev/filemon gets the lower number, 
and no crash.  But no guarantee, either!



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

[David Holland]

On Thu, Jan 07, 2016 at 07:34:33AM +0800, Paul Goyette wrote:
 > 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.)

Looking at the filemon code... it is completely wrong. It should not
be using file handles like that, especially not in a device driver.
Rather than adding new hacks to the system to work around it being
silly, it should be redone properly.

For an example of the right way to do this kind of thing, look in
kern_acct.c.

-- 
David A. Holland
dholl...@netbsd.org

Re: In-kernel process exit hooks?

[David Holland]

On Fri, Jan 08, 2016 at 11:22:28AM +0800, Paul Goyette wrote:
 > Yeah, I was trying to avoid the change in semantics.  :)  The fewer things
 > I touch, the fewer things will go wrong, and I definitely don't want to
 > break make, which would result in difficulties making[sic] a new version.
 > :)

The affected code in make is only sjg's meta-mode thing, which nobody
besides him really understands what it does. So while breaking it
isn't desirable, the risk of getting tangled in it yourself is
negligible.

-- 
David A. Holland
dholl...@netbsd.org

Re: amd64 profiling kernel build failure

[David Holland]

On Fri, Jan 08, 2016 at 02:33:58PM +0900, Kengo NAKAHARA wrote:
 > --- a/sys/kern/subr_prof.c
 > +++ b/sys/kern/subr_prof.c
 > @@ -48,6 +48,10 @@ __KERNEL_RCSID(0, "$NetBSD: subr_prof.c,v 1.47 2014/07/10 
 > 21:13:52 christos Exp
 >  #include 
 >  #include 
 >  
 > +#ifdef MULTIPROCESSOR
 > +__cpu_simple_lock_t __mcount_lock;
 > +#endif
 > +

This should be in an MD file. Not sure offhand which one.

-- 
David A. Holland
dholl...@netbsd.org

Re: In-kernel process exit hooks?

[Robert Elz]

Date:Fri, 8 Jan 2016 11:22:28 +0800 (PHT)
From:Paul Goyette 
Message-ID:  

  | Is there a "supported" interface for detaching the file (or descriptor) 
  | from the process without closing it?

Inside the kernel you want to follow the exact same procedure as would
be done by

newfd = dup(oldfd);
close(oldfd);

except instead of dup (and assigning to a newfd in the process) we
take the file reference and stick it in filemon.   There's nothing
magic about this step.  What magic there is (though barely worthy of
the title) would be in ensuring that filemon properly releases the file
when it is closing.

  | I expect that make(1) assumes the log-fd is still available, and just 
  | follows the example code in filemon(4) man page: when finished logging, 
  | reset the file-pointer to 0 and start reading the data.

make(1) can be changed...   After all, it was added originally, and that
took code changes to make happen.   Further, make must work OK without it,
none of my kernels have filemon included (and they don't load modules),
and make works just fine.

  | Yeah, I was trying to avoid the change in semantics.  :)

Generally a good thing, and if there's a clean way to make it happen,
worth doing.   But this one is a case where that almost certainly doesn't
really matter.

kre

Re: In-kernel process exit hooks?

[Robert Elz]

Date:Fri, 8 Jan 2016 11:22:28 +0800 (PHT)
From:Paul Goyette 
Message-ID:  

  | Is there a "supported" interface for detaching the file (or descriptor) 
  | from the process without closing it?

Actually, thinking through this more, why not just "fix" filemon to make
a proper reference to the file, instead of the half baked thing it is
currently doing.

That is, instead of a fd_getfile() without an (almost immediate) fd_putfile()
so keeping ff_refcount incremented, in filemon, just do

fp = fd_getfile(...);
fp->f_count++;
fd_putfile(...);

so filemon has a proper reference to the file*.   When it is done, it
just closes it, like any other file would be closed (which decrements
f_count and frees the struct file if it goes to 0).

Wouldn't this solve all the problems, keep the semantics the same, and
just generally be cleaner?

Is there some particular benefit filemon gets (aside from a little less
bookkeeping work) by sticking to its half baked reference grabbing scheme?

kre

Re: In-kernel process exit hooks?

[Paul Goyette]

Hmmm, option #3 doesn't work so well, after all!

I tried it when both the target and monitor processes were children of 
the same shell process.  Thus all three of these share the same 'struct 
file' for their stdout, and the filemon_fd_close() callback cannot tell 
which one is being closed.  The all have the same fd (just in different 
descriptor tables?)


So next I'm going to try kre's suggestion about using a "proper" refcnt 
mechanism and see if that helps.


This filemon thing is a real beast!  :)



On Fri, 8 Jan 2016, Paul Goyette wrote:


OK, I now have a third way of handling the problem.

To recap the three options (refer to the attachments)

1. fd_getfile

  This option calls fd_getfile() each time it needs to access the
  activity log, and calls fd_putfile() after each use.  This way,
  the additional reference on the file descriptor lasts only for a
  short period of time, and does not exist at any time that it can
  be passed to fd_close().

  The biggest drawback here is that the user-land application can
  close() and re-open() this fd, possibly referring to a different
  file; this will not be visible to filemon, which will write new
  event records to any file that happens to be opened on this fd.

2. exithook

  This option extends the existing exithook() mechanism to have
  multiple "phases", one of which can happen before the process
  exit code calls fd_free() (which in turn calls fd_close() for all
  open file descriptors).  The exithook registered by filemon finds
  any usages of filemon and resets the activity-log, which releases
  the extra reference to the log fd.

  This option works well for normal process exit (including signal),
  but does not resolve the problem if the application itself calls
  close() on the log fd.  In that situation, the process will still
  hang.

  Additionally, setting this up correctly is awkward, due to the
  order in which kernel components are initialized.  (Modules of
  CLASS_DRIVER get loaded and initialized before exec_init() can
  set up the hook mechanisms, so we need to use a config_finalizer
  to establish the exit hook.)

3. filemon-fd_close

  This solution introduces a new, filemon-specific callback in
  fd_close() (but only if the filemon module is loaded or built-in).
  Each time a file descriptor is passed to fd_close, the callback
  is invoked.  The callback checks each usage of /dev/filemon and
  if that usage is logging activity to the file being closed, the
  activity-log is reset, releasing the extra reference.  Thus,
  after we return to fd_close() the reference count is normal and
  the file gets properly closed.

  The only drawback I see here is the additional overhead of the
  callback, on every call to fd_close().  The code catches every
  occurrence of the "hang" that I can find, and handles it cleanly.

I still need to think about the fd_getfile2()/fclose() approach to see if it 
meets our needs, but the comments that prohibit calling fork() would seem to 
preclude this mechanism.


The "detach file from userland" approach suggested by kre would also likely 
work, but I'm reluctant to change the semantics of filemon.






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

!DSPAM:568f3f46191303231618539!



+--+--++
| 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: amd64 profiling kernel build failure

[NAKAHARA Kengo]

Hi,

On 2016/01/08 15:50, David Holland wrote:

On Fri, Jan 08, 2016 at 02:33:58PM +0900, Kengo NAKAHARA wrote:
  > --- a/sys/kern/subr_prof.c
  > +++ b/sys/kern/subr_prof.c
  > @@ -48,6 +48,10 @@ __KERNEL_RCSID(0, "$NetBSD: subr_prof.c,v 1.47 
2014/07/10 21:13:52 christos Exp
  >  #include 
  >  #include 
  >
  > +#ifdef MULTIPROCESSOR
  > +__cpu_simple_lock_t __mcount_lock;
  > +#endif
  > +

This should be in an MD file. Not sure offhand which one.


Thank you for your comment. I reconsider the patch.


Thanks,

--
//
Internet Initiative Japan Inc.

Device Engineering Section,
Core Product Development Department,
Product Division,
Technology Unit

Kengo NAKAHARA 

Re: amd64 profiling kernel build failure

[David Holland]

On Fri, Jan 08, 2016 at 06:50:02AM +, David Holland wrote:
 >  > --- a/sys/kern/subr_prof.c
 >  > +++ b/sys/kern/subr_prof.c
 >  > @@ -48,6 +48,10 @@ __KERNEL_RCSID(0, "$NetBSD: subr_prof.c,v 1.47 
 > 2014/07/10 21:13:52 christos Exp
 >  >  #include 
 >  >  #include 
 >  >  
 >  > +#ifdef MULTIPROCESSOR
 >  > +__cpu_simple_lock_t __mcount_lock;
 >  > +#endif
 >  > +
 > 
 > This should be in an MD file. Not sure offhand which one.

Also, the i386 profile.h needs the same change as the amd64 one, so
the md file should probably be one in arch/x86/x86.

-- 
David A. Holland
dholl...@netbsd.org

Re: In-kernel process exit hooks?

[David Holland]

On Fri, Jan 08, 2016 at 07:08:19AM +, David Holland wrote:
 > For an example of the right way to do this kind of thing, look in
 > kern_acct.c.

Better example: sys_fktrace, since that uses a file handle. And it
does virtually the same thing that filemon's trying to do, except much
more thoroughly.

-- 
David A. Holland
dholl...@netbsd.org

Re: In-kernel process exit hooks?

[Paul Goyette]

On Thu, 7 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.

Is it supposed to monitor file activity past a fork and exec and
recursively through all subprocesses, or is it supposed to apply only
to the current process?

If it's supposed to work across fork/exec/, then this won't work --
but neither will the current approach!  I think fd_putfile will just
close whatever random descriptor happens to be in that slot, which may
be unrelated if userland (perhaps in some deeply nested child) did
dup2.


What I tried was to use

fd_getfile(fd)
write activity record
fd_putfile(fd)

It seemed to work, but thinking back on it, I have no idea which 
process's descriptor was being used for the lookup - probably curproc 
which would not be correct.  (It would need to be the process which has 
/dev/filemon open.)



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

The descriptor could be closed but the file will persist.


Hmmm. I will have to consider this option.


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