Re: kern/50627: filemon can hang the process

[Paul Goyette <paul%whooppee.com@localhost>]

The following reply was made to PR kern/50627; it has been noted by GNATS.

From: Paul Goyette <paul%whooppee.com@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: gnats-admin%netbsd.org@localhost
Subject: Re: kern/50627: filemon can hang the process
Date: Wed, 6 Jan 2016 19:12:12 +0800 (PHT)

   This message is in MIME format.  The first part should be readable text,
   while the remaining parts are likely unreadable without MIME-aware tools.
 
 --0-476036076-1452078732=:19497
 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed
 
 On Wed, 6 Jan 2016, Martin Husemann wrote:
 
 > The following reply was made to PR kern/50627; it has been noted by GNATS.
 >
 > From: Martin Husemann <martin%duskware.de@localhost>
 > To: gnats-bugs%NetBSD.org@localhost
 > Cc:
 > Subject: Re: kern/50627: filemon can hang the process
 > Date: Wed, 6 Jan 2016 11:26:38 +0100
 >
 > On Wed, Jan 06, 2016 at 06:40:01AM +0000, Robert Elz wrote:
 > >  In-kernel atexit() could solve this problem, and could also potentially
 > >  have uses for other problems, but when things get complex (I don't know
 > >  if it is possible, but consider a process using filemon twice, where one
 > >  of them is logging changes to a file monitored by the other) the atexit()
 > >  order processing is just going to be the same problem all over again -
 > >  something will need to make sure those get done in the correct order,
 > >  which is not necessarily either establishment order, or the reverse of that.
 >
 > I haven't followed the details, so this may be a stupid suggestion: isn't
 > it enough to do the closing loop several times, using cv_timedwait(.., 1)
 > and ignoring the fds where it returns EWOULDBLOCK, untill we did not close
 > any fds in one pass and then switch to cv_wait() in the next pass?
 
 You might be able to get that to work, but not easily.  One of the first 
 thing that happens in fd_close() is to remove the entry from the array 
 so that no new references can be made.  If you use cv_timedwait() and 
 retry on EWOULDBLOCK, you'd have to put the entry back in the table (so 
 it would be seen on the next pass through the loop), and that would 
 allow other code to establish new references.
 
 It could get pretty messy.
 
 
 filemon(4) really has a fairly limited use-case, and it already abuses 
 system interfaces by hijacking/wrapping system calls (which already 
 introduces some ordering dependencies if there were ever any other 
 hijacker).  I'm not sure we want to invent complicated mechanisms to 
 handle this situation.
 
 I'm much more comfortable with making the attached patch (already posted 
 to tech-kern, I think!) to ensure that the fd numbers are in the proper 
 sequence.  The caller can easily use dup(2) or fcntl(2) to get a more 
 acceptable fd number for the log file.
 
 
 
 >
 > Martin
 >
 >
 > !DSPAM:568cecaf221711927294789!
 >
 >
 
 +------------------+--------------------------+------------------------+
 | 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 |
 +------------------+--------------------------+------------------------+
 --0-476036076-1452078732=:19497
 Content-Type: TEXT/PLAIN; charset=US-ASCII; name=filemon.c.diff
 Content-Transfer-Encoding: BASE64
 Content-ID: <Pine.NEB.4.64.1601061912120.19497%pokey.whooppee.com@localhost>
 Content-Description: 
 Content-Disposition: attachment; filename=filemon.c.diff
 
 SW5kZXg6IGZpbGVtb24uYw0KPT09PT09PT09PT09PT09PT09PT09PT09PT09
 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQ0KUkNT
 IGZpbGU6IC9jdnNyb290L3NyYy9zeXMvZGV2L2ZpbGVtb24vZmlsZW1vbi5j
 LHYNCnJldHJpZXZpbmcgcmV2aXNpb24gMS4yNA0KZGlmZiAtdSAtcCAtcjEu
 MjQgZmlsZW1vbi5jDQotLS0gZmlsZW1vbi5jCTUgSmFuIDIwMTYgMjI6MDg6
 NTQgLTAwMDAJMS4yNA0KKysrIGZpbGVtb24uYwk2IEphbiAyMDE2IDA1OjA1
 OjA0IC0wMDAwDQpAQCAtMjc5LDggKzI3OSwxMiBAQCBzdGF0aWMgaW50DQog
 ZmlsZW1vbl9pb2N0bChzdHJ1Y3QgZmlsZSAqIGZwLCB1X2xvbmcgY21kLCB2
 b2lkICpkYXRhKQ0KIHsNCiAJaW50IGVycm9yID0gMDsNCisJaW50IGksIG5m
 LCBmZDsNCiAJc3RydWN0IGZpbGVtb24gKmZpbGVtb247DQogCXN0cnVjdCBw
 cm9jICp0cDsNCisJZmRmaWxlX3QgKmZmOw0KKwlmaWxlZGVzY190ICpmZHA7
 DQorCWZkdGFiX3QgKmR0Ow0KIA0KICNpZmRlZiBERUJVRw0KIAlsb2cobG9n
 TGV2ZWwsICJmaWxlbW9uX2lvY3RsKCVsdSkiLCBjbWQpOzsNCkBAIC0zMDMs
 OCArMzA3LDQxIEBAIGZpbGVtb25faW9jdGwoc3RydWN0IGZpbGUgKiBmcCwg
 dV9sb25nIGMNCiAJCWlmIChmaWxlbW9uLT5mbV9mcCkNCiAJCQlmZF9wdXRm
 aWxlKGZpbGVtb24tPmZtX2ZkKTsNCiANCisJCS8qDQorCQkgKiBYWFggSEFD
 SyBYWFgNCisJCSAqDQorCQkgKiBEdWUgdG8gb3VyIHRha2luZyBhbiBleHRy
 YSByZWZlcmVuY2Ugb24gdGhlDQorCQkgKiBkZXNjcmlwdG9yJ3Mgc3RydWN0
 IGZpbGUsIHdlIG5lZWQgdG8gZW5zdXJlIHRoYXQNCisJCSAqIHRoZSBkZXNj
 cmlwdG9yIG51bWJlciBpcyBhdCBsZWFzdCBhcyBsYXJnZSBhcw0KKwkJICog
 dGhlIG9uZSB1c2VkIHRvIGFjY2VzcyAvZGV2L2ZpbGVtb24uICBPdGhlcndp
 c2UsDQorCQkgKiB3ZSBnZXQgYSBkZWFkbG9jayBkdXJpbmcgcHJvY2VzcyBl
 eGl0LCB3YWl0aW5nDQorCQkgKiBmb3IgdGhlIG91dHB1dCBmaWxlJ3MgcmVm
 ZXJlbmNlIGNvdW50Lg0KKwkJICovDQorCQlmZCA9ICooKGludCAqKSBkYXRh
 KTsNCisJCWZkcCA9IGN1cnByb2MtPnBfZmQ7DQorCQlkdCA9IGN1cmx3cC0+
 bF9mZC0+ZmRfZHQ7DQorCQluZiA9IGR0LT5kdF9uZmlsZXM7DQorCQllcnJv
 ciA9IEVJTlZBTDsNCisJCWZvciAoaSA9IDA7IGkgPCBuZjsgaSsrKSB7DQor
 CQkJaWYgKGkgPj0gZmQpDQorCQkJCWJyZWFrOw0KKwkJCWZmID0gZHQtPmR0
 X2ZmW2ldOw0KKwkJCUtBU1NFUlQoZmQgPj0gTkRGREZJTEUgfHwNCisJCQkJ
 ZmYgPT0gKGZkZmlsZV90ICopZmRwLT5mZF9kZmRmaWxlW2ldKTsNCisJCQlp
 ZiAoZmYgPT0gTlVMTCkNCisJCQkJY29udGludWU7DQorDQorCQkJaWYgKGZm
 LT5mZl9maWxlLT5mX3R5cGUgPT0gRFRZUEVfTUlTQyAmJg0KKwkJCSAgICBm
 Zi0+ZmZfZmlsZS0+Zl9vcHMgPT0gJmZpbGVtb25fZmlsZW9wcykgew0KKwkJ
 CQllcnJvciA9IDA7DQorCQkJCWJyZWFrOw0KKwkJCX0NCisJCX0NCisJCWlm
 IChlcnJvcikNCisJCQlicmVhazsNCisNCiAJCS8qIE5vdyBzZXQgdXAgdGhl
 IG5ldyBvbmUgKi8NCi0JCWZpbGVtb24tPmZtX2ZkID0gKigoaW50ICopIGRh
 dGEpOw0KKwkJZmlsZW1vbi0+Zm1fZmQgPSBmZDsNCiAJCWlmICgoZmlsZW1v
 bi0+Zm1fZnAgPSBmZF9nZXRmaWxlKGZpbGVtb24tPmZtX2ZkKSkgPT0gTlVM
 TCkgew0KIAkJCWVycm9yID0gRUJBREY7DQogCQkJYnJlYWs7DQo=
 
 --0-476036076-1452078732=:19497--