Re: In-kernel process exit hooks?

[Paul Goyette]

On Fri, 8 Jan 2016, Paul Goyette wrote:


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.


Robert Elz wrote:


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?


Option 4 works!  See attached diffs.  Thanks, kre!


I'll commit this in a day or two unless I receive some extremely 
negative feedback.  I know that filemon(4) is not the best code, and it 
really needs a total redesign and rewrite, but that's going to take a 
fair amount of time.  Until then, I think with these changes that we'll 
have a safe-and-stable implementation.  (And if it is decided to remove 
the current code completely, at least we can have a safe-and-stable copy 
in the attic!)



+--+--++
| 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   8 Jan 2016 06:39:58 -
@@ -222,7 +222,6 @@ filemon_open(dev_t dev, int oflags __unu
 
filemon = kmem_alloc(sizeof(struct filemon), KM_SLEEP);
rw_init(>fm_mtx);
-   filemon->fm_fd = -1;
filemon->fm_fp = NULL;
filemon->fm_pid = curproc->p_pid;
 
@@ -265,7 +264,7 @@ filemon_close(struct file * fp)
 */
rw_enter(>fm_mtx, RW_WRITER);
if (filemon->fm_fp) {
-   fd_putfile(filemon->fm_fd); /* release our reference */
+   closef(filemon->fm_fp); /* release our reference */
filemon->fm_fp = NULL;
}
rw_exit(>fm_mtx);
@@ -279,6 +278,7 @@ static int
 filemon_ioctl(struct file * fp, u_long cmd, void *data)
 {
int error = 0;
+   int fd;
struct filemon *filemon;
struct proc *tp;
 
@@ -301,14 +301,16 @@ filemon_ioctl(struct file * fp, u_long c
 
/* First, release any current output file descriptor */
if (filemon->fm_fp)
-   fd_putfile(filemon->fm_fd);
+   closef(filemon->fm_fp);
 
/* Now set up the new one */
-   filemon->fm_fd = *((int *) data);
-   if ((filemon->fm_fp = fd_getfile(filemon->fm_fd)) == NULL) {
+   fd = *((int *) data);
+   if ((filemon->fm_fp = fd_getfile(fd)) == NULL) {
error = EBADF;
break;
}
+   filemon->fm_fp->f_count++;
+   fd_putfile(fd);
/* Write the file header. */
filemon_comment(filemon);
break;
Index: filemon.h
===
RCS file: /cvsroot/src/sys/dev/filemon/filemon.h,v
retrieving revision 1.8
diff -u -p -r1.8 filemon.h
--- filemon.h   25 Nov 2015 07:34:49 -  1.8
+++ filemon.h   8 Jan 2016 06:39:58 -
@@ -41,7 +41,6 @@ struct filemon {
char fm_fname1[MAXPATHLEN];/* Temporary filename buffer. */
char fm_fname2[MAXPATHLEN];/* Temporary filename buffer. */
char fm_msgbufr[32 + 2 * MAXPATHLEN];   /* Output message buffer. */
-   int fm_fd;  /* Output fd */
struct file *fm_fp; /* Output file pointer. */
krwlock_t fm_mtx;   /* Lock mutex for this filemon. */
TAILQ_ENTRY(filemon) fm_link;   /* Link into the in-use list. */

Re: amd64 profiling kernel build failure

[Kengo NAKAHARA]

Hi,

On 2016/01/08 16:00, David Holland wrote:
> 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.

I update the patch. I define "__mcount_lock" in the new file
sys/arch/x86/x86/profile.c. Here is the patch.

diff --git a/sys/arch/amd64/include/profile.h b/sys/arch/amd64/include/profile.h
index f760594..24ea606 100644
--- a/sys/arch/amd64/include/profile.h
+++ b/sys/arch/amd64/include/profile.h
@@ -85,7 +85,7 @@ __asm(" .globl __mcount   \n" 
\
 
 #ifdef _KERNEL
 #ifdef MULTIPROCESSOR
-__cpu_simple_lock_t __mcount_lock;
+extern __cpu_simple_lock_t __mcount_lock;
 
 static inline void
 MCOUNT_ENTER_MP(void)
diff --git a/sys/arch/i386/include/profile.h b/sys/arch/i386/include/profile.h
index d49e95a..7ac9b4f 100644
--- a/sys/arch/i386/include/profile.h
+++ b/sys/arch/i386/include/profile.h
@@ -84,7 +84,7 @@ mcount(void)  
\
 
 #ifdef _KERNEL
 #ifdef MULTIPROCESSOR
-__cpu_simple_lock_t __mcount_lock;
+extern __cpu_simple_lock_t __mcount_lock;
 
 static inline void
 MCOUNT_ENTER_MP(void)
diff --git a/sys/arch/x86/conf/files.x86 b/sys/arch/x86/conf/files.x86
index 2edb65f..4911c35 100644
--- a/sys/arch/x86/conf/files.x86
+++ b/sys/arch/x86/conf/files.x86
@@ -28,6 +28,8 @@ devicecpu: cpufeaturebus
 attach cpu at cpubus
 file   arch/x86/x86/cpu.c  cpu
 
+file   arch/x86/x86/profile.c
+
 device acpicpu: acpi
 attach acpicpu at cpufeaturebus
 file   dev/acpi/acpi_cpu.c acpicpu
diff --git a/sys/arch/x86/x86/profile.c b/sys/arch/x86/x86/profile.c
new file mode 100644
index 000..9d554fb
--- /dev/null
+++ b/sys/arch/x86/x86/profile.c
@@ -0,0 +1,38 @@
+/* $NetBSD$*/
+
+/*
+ * Copyright (c) 2016 Internet Initiative Japan Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *notice, this list of conditions and the following disclaimer in the
+ *documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE NETBSD FOUNDATION, INC. AND CONTRIBUTORS
+ * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE FOUNDATION OR CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include 
+__KERNEL_RCSID(0, "$NetBSD$");
+
+#include "opt_multiprocessor.h"
+
+#include 
+
+#ifdef MULTIPROCESSOR
+__cpu_simple_lock_t __mcount_lock;
+#endif


Could you comment this patch? Any comments are welcome.


Thanks,

-- 
//
Internet Initiative Japan Inc.

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

Kengo NAKAHARA 

Re: In-kernel process exit hooks?

[bch]

On 1/7/16, David Holland  wrote:
> 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.

Out of curiousity, I'm trying to use this interface, but getting:

/usr/include/sys/ktrace.h:83:20: error: field '_ts' has incomplete type
struct timespec _ts;

Does this look familiar to anybody ?

I also needed to manually include  for MAXCOMLEN (used in
 L66)


Cheers,

-bch



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

Re: In-kernel process exit hooks?

[Paul Goyette]

On Fri, 8 Jan 2016, bch wrote:


On 1/7/16, David Holland  wrote:

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.


Out of curiousity, I'm trying to use this interface, but getting:

/usr/include/sys/ktrace.h:83:20: error: field '_ts' has incomplete type
   struct timespec _ts;

Does this look familiar to anybody ?


Perhaps manually include  ?



I also needed to manually include  for MAXCOMLEN (used in
 L66)


Cheers,

-bch




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



!DSPAM:568f7d53105566048743086!




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

[bch]

On 1/8/16, Paul Goyette  wrote:
> On Fri, 8 Jan 2016, bch wrote:
>
>> On 1/7/16, David Holland  wrote:
>>> 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.
>>
>> Out of curiousity, I'm trying to use this interface, but getting:
>>
>> /usr/include/sys/ktrace.h:83:20: error: field '_ts' has incomplete type
>>struct timespec _ts;
>>
>> Does this look familiar to anybody ?
>
> Perhaps manually include  ?

That's it.


Thanks.

>> I also needed to manually include  for MAXCOMLEN (used in
>>  L66)
>>
>>
>> Cheers,
>>
>> -bch
>>
>>
>>
>>> --
>>> David A. Holland
>>> dholl...@netbsd.org
>>>
>>
>> !DSPAM:568f7d53105566048743086!
>>
>>
>
> +--+--++
> | 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 Young]

On Fri, Jan 08, 2016 at 12:52:16PM +0700, Robert Elz wrote:
> 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.

Yes, please! :-)

Furthermore, stick the file into LWP 0's descriptor table so that you
can see it with fstat.  It's a little more code to write---I wrote it
for gre(4)---but it's well worth the visibility.

Dave

-- 
David Young
dyo...@pobox.comUrbana, IL(217) 721-9981

Re: amd64 profiling kernel build failure

[Christos Zoulas]

In article <20160108173359.a539f1cc...@yaml.nerv.org>,
Ryo Shimizu   wrote:
>
>Hi all
>
>>Hi,
>>
>>On 2016/01/08 16:00, David Holland wrote:
>>> 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.
>
>BTW, as far as I know other MULTIPROCESSOR arch also needs
>__mcount_lock, but none.
>At least kernel profiling doesn't work on arm/MP.
>
>therefore __mcount_lock should be moved to common/lib/libc/gmon/mcount.c
>from machine/profile.h as below.

Go for it I guess.

christos

Re: In-kernel process exit hooks?

[bch]

On 1/8/16, Taylor R Campbell  wrote:
>Date: Fri, 8 Jan 2016 17:13:54 +0800 (PHT)
>From: Paul Goyette 
>
>On Fri, 8 Jan 2016, bch wrote:
>
>> Out of curiousity, I'm trying to use this interface, but getting:
>>
>> /usr/include/sys/ktrace.h:83:20: error: field '_ts' has incomplete
> type
>>struct timespec _ts;
>>
>> Does this look familiar to anybody ?
>
>Perhaps manually include  ?
>
> Shouldn't be necessary.  If  uses struct timespec, it
> should include .
>

http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=50633

http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=50634

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

[Paul Goyette]

On Sat, 9 Jan 2016, Rhialto wrote:


On Wed 06 Jan 2016 at 17:44:45 +, Taylor R Campbell wrote:

This only fixes the problem for certain orderings of file descriptors.


I was thinking of a different hack.

Given tha filemon now knows there are issues if it has to use a fd lower
than its own fd, it can avoid the situation.

If it happens, it might dup2() the output fd so that it gets one that is
high enough, to use instead. That ought to work, since as I understand
the issue is references to the file descriptor, not references to the
file structure.

Of course I can immediately see some disadvantages to this apprroach:

- this will use an extra fd and that is observable by the process
- and the process might even close that fd if it is doing some blanket
 close-all-fds action.

Maybe these potential issues can be avoided somehow?


Yes, we avoid all these issues by taking the reference on the file 
itself, rather than on the descriptor.



+--+--++
| 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 Fri, 8 Jan 2016, Taylor R Campbell wrote:


  Date: Fri, 8 Jan 2016 16:52:28 +0800 (PHT)
  From: Paul Goyette 

  Robert Elz wrote:

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

  Option 4 works!  See attached diffs.  Thanks, kre!

This is essentially the same as fd_getfile2, except with a lot more
logic involved, including fiddly details that you have to take care
of.  And...

  I'll commit this in a day or two unless I receive some extremely
  negative feedback.

  @@ -301,14 +301,16 @@ filemon_ioctl(struct file * fp, u_long c
  [...]
  -   filemon->fm_fd = *((int *) data);
  -   if ((filemon->fm_fp = fd_getfile(filemon->fm_fd)) == NULL) {
  +   fd = *((int *) data);
  +   if ((filemon->fm_fp = fd_getfile(fd)) == NULL) {
  error = EBADF;
  break;
  }
  +   filemon->fm_fp->f_count++;
  +   fd_putfile(fd);

...you missed one: you can't touch fp->f_count without holding
fp->f_lock.

So I reiterate my suggestion to use fd_getfile2:

fd = *((int *)data);
if ((filemon->fm_fp = fd_getfile2(curproc, fd)) == NULL) {
error = EBADF;
break;
}


Ack - let me test and confirm that it works.



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

[Rhialto]

On Wed 06 Jan 2016 at 17:44:45 +, Taylor R Campbell wrote:
> This only fixes the problem for certain orderings of file descriptors.

I was thinking of a different hack.

Given tha filemon now knows there are issues if it has to use a fd lower
than its own fd, it can avoid the situation.

If it happens, it might dup2() the output fd so that it gets one that is
high enough, to use instead. That ought to work, since as I understand
the issue is references to the file descriptor, not references to the
file structure.

Of course I can immediately see some disadvantages to this apprroach:

- this will use an extra fd and that is observable by the process
- and the process might even close that fd if it is doing some blanket
  close-all-fds action.

Maybe these potential issues can be avoided somehow?

-Olaf.
-- 
___ Olaf 'Rhialto' Seibert  -- The Doctor: No, 'eureka' is Greek for
\X/ rhialto/at/xs4all.nl-- 'this bath is too hot.'


signature.asc
Description: PGP signature

Re: In-kernel process exit hooks?

[Paul Goyette]

On Fri, 8 Jan 2016, Taylor R Campbell wrote:


  > 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 don't think that's what it means.  I think it means the process
whose descriptor we're asking for can't fork or exit *during the call*
to fd_getfile2.  In this case, it's the current process, so that's not
an issue.

The reason it is an issue stated in a comment is that the routine
appears to have been written for the purpose of procfs, in which case
the calling process is often not the same as the process passed to
fd_getfile2.


Ah, OK, that makes sense.




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

On Fri, Jan 08, 2016 at 10:47:08AM -0600, David Young wrote:
> On Fri, Jan 08, 2016 at 12:52:16PM +0700, Robert Elz wrote:
> > 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.
> 
> Yes, please! :-)
> 
> Furthermore, stick the file into LWP 0's descriptor table so that you

Oops, meant PID 0's.

Dave

-- 
David Young
dyo...@pobox.comUrbana, IL(217) 721-9981

Re: amd64 profiling kernel build failure

[Ryo Shimizu]

Hi all

>Hi,
>
>On 2016/01/08 16:00, David Holland wrote:
>> 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.

BTW, as far as I know other MULTIPROCESSOR arch also needs __mcount_lock, but 
none.
At least kernel profiling doesn't work on arm/MP.

therefore __mcount_lock should be moved to common/lib/libc/gmon/mcount.c
from machine/profile.h as below.


diff --git a/common/lib/libc/gmon/mcount.c b/common/lib/libc/gmon/mcount.c
index 128abe6..bf50572 100644
--- a/common/lib/libc/gmon/mcount.c
+++ b/common/lib/libc/gmon/mcount.c
@@ -82,6 +82,7 @@ __RCSID("$NetBSD: mcount.c,v 1.10 2012/03/20 16:21:41 matt 
Exp $");
 
 #include 
 #include 
+#include 
 
 #ifndef _KERNEL
 #include "reentrant.h"
@@ -93,6 +94,10 @@ extern struct gmonparam _gmondummy;
 struct gmonparam *_m_gmon_alloc(void);
 #endif
 
+#if defined(_KERNEL) && !defined(_RUMPKERNEL) && defined(MULTIPROCESSOR)
+__cpu_simple_lock_t __mcount_lock;
+#endif
+
 #ifndef __LINT__
 _MCOUNT_DECL(u_long, u_long)
 #ifdef _KERNEL
@@ -101,16 +106,6 @@ _MCOUNT_DECL(u_long, u_long)
 __used;
 #endif
 
-/* XXX: make these interfaces */
-#ifdef _RUMPKERNEL
-#undef MCOUNT_ENTER
-#define MCOUNT_ENTER
-#undef MCOUNT_EXIT
-#define MCOUNT_EXIT
-#undef MCOUNT
-#define MCOUNT
-#endif
-
 /*
  * mcount is called on entry to each function compiled with the profiling
  * switch set.  _mcount(), which is declared in a machine-dependent way
@@ -155,8 +150,12 @@ _MCOUNT_DECL(u_long frompc, u_long selfpc)
 */
if (p->state != GMON_PROF_ON)
return;
-#ifdef _KERNEL
+#if defined(_KERNEL) && !defined(_RUMPKERNEL)
MCOUNT_ENTER;
+#ifdef MULTIPROCESSOR
+   __cpu_simple_lock(&__mcount_lock);
+   __insn_barrier();
+#endif
 #endif
p->state = GMON_PROF_BUSY;
/*
@@ -250,7 +249,11 @@ _MCOUNT_DECL(u_long frompc, u_long selfpc)
}
 done:
p->state = GMON_PROF_ON;
-#ifdef _KERNEL
+#if defined(_KERNEL) && !defined(_RUMPKERNEL)
+#ifdef MULTIPROCESSOR
+   __insn_barrier();
+   __cpu_simple_unlock(&__mcount_lock);
+#endif
MCOUNT_EXIT;
 #endif
return;
diff --git a/sys/arch/amd64/include/profile.h b/sys/arch/amd64/include/profile.h
index f760594..3ff4d2d 100644
--- a/sys/arch/amd64/include/profile.h
+++ b/sys/arch/amd64/include/profile.h
@@ -34,7 +34,6 @@
 #ifdef __x86_64__
 
 #ifdef _KERNEL_OPT
-#include "opt_multiprocessor.h"
 #include "opt_xen.h"
 #endif
 
@@ -84,27 +83,6 @@ __asm(" .globl __mcount  \n" 
\
 
 
 #ifdef _KERNEL
-#ifdef MULTIPROCESSOR
-__cpu_simple_lock_t __mcount_lock;
-
-static inline void
-MCOUNT_ENTER_MP(void)
-{
-   __cpu_simple_lock(&__mcount_lock);
-   __insn_barrier();
-}
-
-static inline void
-MCOUNT_EXIT_MP(void)
-{
-   __insn_barrier();
-   __mcount_lock = __SIMPLELOCK_UNLOCKED;
-}
-#else
-#define MCOUNT_ENTER_MP()
-#define MCOUNT_EXIT_MP()
-#endif
-
 #ifdef XEN
 static inline void
 mcount_disable_intr(void)
@@ -150,14 +128,10 @@ mcount_write_psl(u_long ef)
 }
 
 #endif /* XEN */
-#defineMCOUNT_ENTER
\
-   s = (int)mcount_read_psl(); \
-   mcount_disable_intr();  \
-   MCOUNT_ENTER_MP();
-
-#defineMCOUNT_EXIT 
\
-   MCOUNT_EXIT_MP();   \
-   mcount_write_psl(s);
+
+#define MCOUNT_ENTER   \
+   do { s = (int)mcount_read_psl(); mcount_disable_intr(); } while (0)
+#define MCOUNT_EXITdo { mcount_write_psl(s); } while (0)
 
 #endif /* _KERNEL */
 
diff --git a/sys/arch/i386/include/profile.h b/sys/arch/i386/include/profile.h
index d49e95a..923aa33 100644
--- a/sys/arch/i386/include/profile.h
+++ b/sys/arch/i386/include/profile.h
@@ -31,13 +31,8 @@
  * @(#)profile.h   8.1 (Berkeley) 6/11/93
  */
 
-#ifdef _KERNEL_OPT
-#include "opt_multiprocessor.h"
-#endif
-
 #ifdef _KERNEL
 #include 
-#include 
 #endif
 
 #define_MCOUNT_DECL static __inline void _mcount
@@ -83,27 +78,6 @@ mcount(void) 
\
 }
 
 #ifdef _KERNEL
-#ifdef MULTIPROCESSOR
-__cpu_simple_lock_t __mcount_lock;
-
-static inline void
-MCOUNT_ENTER_MP(void)
-{
-   __cpu_simple_lock(&__mcount_lock);
-   __insn_barrier();
-}
-
-static inline void
-MCOUNT_EXIT_MP(void)
-{
-   __insn_barrier();
-  

Re: In-kernel process exit hooks?

[Paul Goyette]

On Sat, 9 Jan 2016, Mateusz Guzik wrote:


While I don't know all the details, it is clear that the purpose would
be much better served by ktrace and I would argue efforts should be
spent there.


filemon's purpose is somewhat different than that of ktrace.  There is a 
limited set of events that filemon captures, and it only captures 
successful operations.  Its output is quite simple and easy to parse by 
a human of shell script.



bmake takes filemon output and copies it into some file.


Yes.  In "meta-mode" it will use the output from filemon to capture 
dependency information.



From usability perspective what seems to be needed is few hacks to

ktrace to only log the stuff make is interested in and parsing code for
bmake to translate to what looks like current filemon format for
aformentioned copying.


Perhaps.  But filemon is also a run-time loadable module (and it is 
preferred that it NOT be built-in to any kernels).  If I remember 
correctly, ktrace has hooks all over the system and does not readily 
lend itself to modularization.



I only had a brief look at ktrace implementation. It uses global locks,
so that would have to be worked on, but it should be trivial to at least
make tracees using different output files not compete with each other.

So, what's so fitting about ktrace design: tracing is tied to the target
process, can react to things like changing credentials and reparenting
(fork + parent exit), but most importantly it does not make assumptions
about things which can change without its control (see below).

Filemon monitors stuff at syscall level, stores the fd and remembers
pids.


The fd is no longer being stored...  :)  But yes, it does still remember 
pids, so it can determine if an event belongs to a monitored process 
tree.  Again IIRC, ktrace is somewhat more intrusive, in that it 
manipulates various process flags which can affect behavior of the 
target (monitored) process.



Let's see how this leads to trouble. Some of these problems were also
present in FreeBSD implementation (and some still are). A bonus
interesting fact is that implementations have diverged, although are
still fundamentally broken.

http://nxr.netbsd.org/xref/src/sys/dev/filemon/filemon.c#filemon_ioctl

   case FILEMON_SET_PID:
   /* Set the monitored process ID - if allowed. */
   mutex_enter(proc_lock);
   tp = proc_find(*((pid_t *) data));


There are no steps taken to keep tp around nor in a certain state (see
below). proc_find only finds the pointer.


We really don't need further access to the struct proc...


   mutex_exit(proc_lock);
   if (tp == NULL ||
   tp->p_emul != _netbsd) {


tp could have exited and be freed by now. Now that the comparison was
done it could have p_emul changed.


The mutex_exit() has been moved to after the last use of data which is 
protected by the mutex.



   error = ESRCH;
   break;
   }

   error = kauth_authorize_process(curproc->p_cred,
   KAUTH_PROCESS_CANSEE, tp,
   KAUTH_ARG(KAUTH_REQ_PROCESS_CANSEE_ENTRY), NULL, NULL);


Now that the check is pased the process could have executed something
setuid.


Which process could have executed something setuid?  How would that 
matter?  (I will confess I haven't checked, but I would be surprised if 
"executing something setuid" would change the result of kauth() check. 
But I could be wrong here.)



I'm somewhat surprised this code is here. Should not
kauth_authorize_process assert that tp has stable credentials in some
way, in effect just crashing filemon consumers?


   if (!error) {
   filemon->fm_pid = tp->p_pid;
   }


Why does the target process need stable credentials?  It's not doing 
anything for filemon.



Pid is stored, but tp is not held. What if the 'traced' process exits?
grep only reveals the following bit in filemon_wrapper_sys_exit:


   if (filemon->fm_pid == curproc->p_pid)
   filemon_printf(filemon, "# Bye bye\n");


So filemon will continue tracing anything which happened to reuse the
pid.


Yes, this is an outstanding issue.  More particularly, the new user of 
the pid might not be accessible by the monitoring process (ie, it would 
fail the earlier kauth() check).  But holding the pointer to the proc 
isn't really the answer either - although less likely it is entirely 
possible that the address could have been reused.


(It's too bad we don't have "generation numbers" on the pid, which would 
easily detect reuse.)



"Tracing points" are at syscall layer. With only pid remembered this
adds up to serious breakage induced by the need to look the filemon structure
up each time, and based on unreliable data.


static struct filemon *
filemon_pid_check(struct proc * p)
{
   struct filemon *filemon;
   struct proc * lp;

   if 

Re: In-kernel process exit hooks?

[Paul Goyette]

I'm not going to address individual points

We all agree that filemon(4) is an ugly hack.  It probably should never
have gotten committed.  But it is there now, and there are a (very) few
use-cases.  So we don't want to remove it without having a replacement
implementation.  (Although some of the issues, like not noticing a pid
being resued, could conceivably be considered security-related - ie,
information disclosure of a process to which you don't have access...)

I'll be working on a replacement, but it will take a while to get it
right.  At least several months, perhaps even longer.


make(1) (or bmake) doesn't have to move very far to avoid filemon(4). 
The only usage of filemon within make is for "meta-mode" which is only 
really understood by its author.  Indeed, filemon was created just for 
this one use-case;  any other use of filemon is truly accidental!


OK, I will address just one of your points individually:


The thing is there is filemon and bmake in FreeBSD as well. While I
don't have the time to modify ktrace for this purpose, I would
definitely help both projects if bmake started moving away from
filemon.

However, this is just my $0,03 as I don't contribute to this project.


Nothing prevents you from working on this and contributing any working 
code.  We're all volunteers here.


:)


+--+--++
| 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 Sat, Jan 09, 2016 at 08:25:05AM +0100, Mateusz Guzik wrote:
 > >if (!mutex_tryenter(parent->p_lock)) {
 > >mutex_exit(t->p_lock);
 > >mutex_enter(parent->p_lock);
 > 
 > As a side note this looks like a bug. t->p_lock is not relocked, so the
 > code ends up without the lock held. There is a similar sample in fork1.

I just fixed this. Thanks.

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

Re: In-kernel process exit hooks?

[Paul Goyette]

So I reiterate my suggestion to use fd_getfile2:

fd = *((int *)data);
if ((filemon->fm_fp = fd_getfile2(curproc, fd)) == NULL) {
error = EBADF;
break;
}


Ack - let me test and confirm that it works.


Yes, it works just fine.  Revised diffs are attached.

So, I'll plan on committing this in the next couple of days.  Unless 
there is strong objection raised.




+--+--++
| 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.26
diff -u -p -r1.26 filemon.c
--- filemon.c   8 Jan 2016 08:57:14 -   1.26
+++ filemon.c   9 Jan 2016 04:05:21 -
@@ -222,7 +222,6 @@ filemon_open(dev_t dev, int oflags __unu
 
filemon = kmem_alloc(sizeof(struct filemon), KM_SLEEP);
rw_init(>fm_mtx);
-   filemon->fm_fd = -1;
filemon->fm_fp = NULL;
filemon->fm_pid = curproc->p_pid;
 
@@ -265,7 +264,7 @@ filemon_close(struct file * fp)
 */
rw_enter(>fm_mtx, RW_WRITER);
if (filemon->fm_fp) {
-   fd_putfile(filemon->fm_fd); /* release our reference */
+   closef(filemon->fm_fp); /* release our reference */
filemon->fm_fp = NULL;
}
rw_exit(>fm_mtx);
@@ -279,6 +278,7 @@ static int
 filemon_ioctl(struct file * fp, u_long cmd, void *data)
 {
int error = 0;
+   int fd;
struct filemon *filemon;
struct proc *tp;
 
@@ -301,11 +301,11 @@ filemon_ioctl(struct file * fp, u_long c
 
/* First, release any current output file descriptor */
if (filemon->fm_fp)
-   fd_putfile(filemon->fm_fd);
+   closef(filemon->fm_fp);
 
/* Now set up the new one */
-   filemon->fm_fd = *((int *) data);
-   if ((filemon->fm_fp = fd_getfile(filemon->fm_fd)) == NULL) {
+   fd = *((int *) data);
+   if ((filemon->fm_fp = fd_getfile2(curproc, fd)) == NULL) {
error = EBADF;
break;
}
Index: filemon.h
===
RCS file: /cvsroot/src/sys/dev/filemon/filemon.h,v
retrieving revision 1.8
diff -u -p -r1.8 filemon.h
--- filemon.h   25 Nov 2015 07:34:49 -  1.8
+++ filemon.h   9 Jan 2016 04:05:21 -
@@ -41,7 +41,6 @@ struct filemon {
char fm_fname1[MAXPATHLEN];/* Temporary filename buffer. */
char fm_fname2[MAXPATHLEN];/* Temporary filename buffer. */
char fm_msgbufr[32 + 2 * MAXPATHLEN];   /* Output message buffer. */
-   int fm_fd;  /* Output fd */
struct file *fm_fp; /* Output file pointer. */
krwlock_t fm_mtx;   /* Lock mutex for this filemon. */
TAILQ_ENTRY(filemon) fm_link;   /* Link into the in-use list. */

Re: In-kernel process exit hooks?

[Taylor R Campbell]

   Date: Fri, 8 Jan 2016 16:52:28 +0800 (PHT)
   From: Paul Goyette 

   Robert Elz wrote:

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

   Option 4 works!  See attached diffs.  Thanks, kre!

This is essentially the same as fd_getfile2, except with a lot more
logic involved, including fiddly details that you have to take care
of.  And...

   I'll commit this in a day or two unless I receive some extremely 
   negative feedback.

   @@ -301,14 +301,16 @@ filemon_ioctl(struct file * fp, u_long c
   [...]
   -   filemon->fm_fd = *((int *) data);
   -   if ((filemon->fm_fp = fd_getfile(filemon->fm_fd)) == NULL) {
   +   fd = *((int *) data);
   +   if ((filemon->fm_fp = fd_getfile(fd)) == NULL) {
   error = EBADF;
   break;
   }
   +   filemon->fm_fp->f_count++;
   +   fd_putfile(fd);

...you missed one: you can't touch fp->f_count without holding
fp->f_lock.

So I reiterate my suggestion to use fd_getfile2:

fd = *((int *)data);
if ((filemon->fm_fp = fd_getfile2(curproc, fd)) == NULL) {
error = EBADF;
break;
}

Re: In-kernel process exit hooks?

[David Young]

On Fri, Jan 08, 2016 at 12:26:14PM +0700, Robert Elz wrote:
> 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.

Years ago I added to gre(4) an ioctl that a user thread can use to
delegate to the kernel a UDP socket that carries tunnel traffic.  I
think that that code should cover at least the dup(2) part of Robert's
suggestion.

Dave

-- 
David Young
dyo...@pobox.comUrbana, IL(217) 721-9981

Re: In-kernel process exit hooks?

[Taylor R Campbell]

   Date: Fri, 8 Jan 2016 17:13:54 +0800 (PHT)
   From: Paul Goyette 

   On Fri, 8 Jan 2016, bch wrote:

   > Out of curiousity, I'm trying to use this interface, but getting:
   >
   > /usr/include/sys/ktrace.h:83:20: error: field '_ts' has incomplete type
   >struct timespec _ts;
   >
   > Does this look familiar to anybody ?

   Perhaps manually include  ?

Shouldn't be necessary.  If  uses struct timespec, it
should include .

Re: In-kernel process exit hooks?

[Taylor R Campbell]

   Date: Fri, 8 Jan 2016 09:00:03 +0800 (PHT)
   From: 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 don't think that's what it means.  I think it means the process
whose descriptor we're asking for can't fork or exit *during the call*
to fd_getfile2.  In this case, it's the current process, so that's not
an issue.

The reason it is an issue stated in a comment is that the routine
appears to have been written for the purpose of procfs, in which case
the calling process is often not the same as the process passed to
fd_getfile2.