code review 7870043: runtime: explicitly remove fd's from epoll waitset befo...

runtime: explicitly remove fd's from epoll waitset before close()
Fixes issue 5061.

Current code relies on the fact that fd's are automatically removed from epoll set when closed. However, it is not true. Underlying file description is removed from epoll set only when *all* fd's referring to it are closed.

There are 2 bad consequences:
1. Kernel delivers notifications on already closed fd's.
2. The following sequence of events leads to error:
   - add fd1 to epoll
   - dup fd1 = fd2
   - close fd1 (not removed from epoll since we've dup'ed the fd)
   - dup fd2 = fd1 (get the same fd as fd1)
   - add fd1 to epoll = EEXIST

So, if fd can be potentially dup'ed of fork'ed, it's necessary to explicitly remove the fd from epoll set.

[dvyukov, 2013-03-16 14:46:12]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://dvyukov%40google.com@code.google.com/p/go/

[bradfitz, 2013-03-16 15:54:38]

https://codereview.appspot.com/7870043/diff/4001/src/pkg/runtime/netpoll_epoll.c
File src/pkg/runtime/netpoll_epoll.c (right):

https://codereview.appspot.com/7870043/diff/4001/src/pkg/runtime/netpoll_epol...
src/pkg/runtime/netpoll_epoll.c:42: return -res;
this part (returning negative, where you used to return epollctl's return value
directly) seems unrelated to the rest of the CL.  is this just an additional
fix?

[dvyukov, 2013-03-16 17:18:37]

https://codereview.appspot.com/7870043/diff/4001/src/pkg/runtime/netpoll_epoll.c
File src/pkg/runtime/netpoll_epoll.c (right):

https://codereview.appspot.com/7870043/diff/4001/src/pkg/runtime/netpoll_epol...
src/pkg/runtime/netpoll_epoll.c:42: return -res;
On 2013/03/16 15:54:38, bradfitz wrote:
> this part (returning negative, where you used to return epollctl's return
value
> directly) seems unrelated to the rest of the CL.  is this just an additional
> fix?

The issue describes 2 bugs. One is that the program fails, and the second is
that the error message is "errno -17". This CL fixes both.

[dfc, 2013-03-16 20:47:38]

Thank you Dmitry. Could you please add some text to the description explaining
why this change is ncessary. I'm guessing that there is a bug/misfeature an
epoll that means closing fd race with calls to read and write.

https://codereview.appspot.com/7870043/diff/4001/src/pkg/net/fd_unix.go
File src/pkg/net/fd_unix.go (right):

https://codereview.appspot.com/7870043/diff/4001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:127: fd.pd.Close()
what is the rational for rearranging these lines ? I'm guessing you must call
fd.pd.Close to unregister the fd from any epoll/kqueue structure. 

Please add a comment so they don't get rearranged in the future.

[dvyukov, 2013-03-17 07:49:30]

On 2013/03/16 20:47:38, dfc wrote:
> Thank you Dmitry. Could you please add some text to the description explaining
> why this change is ncessary. I'm guessing that there is a bug/misfeature an
> epoll that means closing fd race with calls to read and write.
> 
> https://codereview.appspot.com/7870043/diff/4001/src/pkg/net/fd_unix.go
> File src/pkg/net/fd_unix.go (right):
> 
>
https://codereview.appspot.com/7870043/diff/4001/src/pkg/net/fd_unix.go#newco...
> src/pkg/net/fd_unix.go:127: fd.pd.Close()
> what is the rational for rearranging these lines ? I'm guessing you must call
> fd.pd.Close to unregister the fd from any epoll/kqueue structure. 
> 
> Please add a comment so they don't get rearranged in the future.

Done. PTAL.

[dfc, 2013-03-17 09:34:18]

Thank you. Given the pd.Close doesn't close anything, is it possible to rename
it to something like Remove or Deregister ?



On 17/03/2013, at 6:49 PM, dvyukov@google.com wrote:

> On 2013/03/16 20:47:38, dfc wrote:
>> Thank you Dmitry. Could you please add some text to the description
> explaining
>> why this change is ncessary. I'm guessing that there is a
> bug/misfeature an
>> epoll that means closing fd race with calls to read and write.
> 
> 
> https://codereview.appspot.com/7870043/diff/4001/src/pkg/net/fd_unix.go
>> File src/pkg/net/fd_unix.go (right):
> 
> 
>
https://codereview.appspot.com/7870043/diff/4001/src/pkg/net/fd_unix.go#newco...
>> src/pkg/net/fd_unix.go:127: fd.pd.Close()
>> what is the rational for rearranging these lines ? I'm guessing you
> must call
>> fd.pd.Close to unregister the fd from any epoll/kqueue structure.
> 
>> Please add a comment so they don't get rearranged in the future.
> 
> Done. PTAL.
> 
> https://codereview.appspot.com/7870043/

[dvyukov, 2013-03-17 09:53:09]

It abstractedly closes poller, the operation can include removing,
unregistering or closing something.


On Sun, Mar 17, 2013 at 1:34 PM, Dave Cheney <dave@cheney.net> wrote:
> Thank you. Given the pd.Close doesn't close anything, is it possible to rename
it to something like Remove or Deregister ?
>
>
>
> On 17/03/2013, at 6:49 PM, dvyukov@google.com wrote:
>
>> On 2013/03/16 20:47:38, dfc wrote:
>>> Thank you Dmitry. Could you please add some text to the description
>> explaining
>>> why this change is ncessary. I'm guessing that there is a
>> bug/misfeature an
>>> epoll that means closing fd race with calls to read and write.
>>
>>
>> https://codereview.appspot.com/7870043/diff/4001/src/pkg/net/fd_unix.go
>>> File src/pkg/net/fd_unix.go (right):
>>
>>
>>
https://codereview.appspot.com/7870043/diff/4001/src/pkg/net/fd_unix.go#newco...
>>> src/pkg/net/fd_unix.go:127: fd.pd.Close()
>>> what is the rational for rearranging these lines ? I'm guessing you
>> must call
>>> fd.pd.Close to unregister the fd from any epoll/kqueue structure.
>>
>>> Please add a comment so they don't get rearranged in the future.
>>
>> Done. PTAL.
>>
>> https://codereview.appspot.com/7870043/

[bradfitz, 2013-03-18 20:56:02]

LGTM

https://codereview.appspot.com/7870043/diff/12001/src/pkg/net/fd_unix.go
File src/pkg/net/fd_unix.go (right):

https://codereview.appspot.com/7870043/diff/12001/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:127: // Poller may want to unregister fd in rediness
notification mechanism,
missing an 'a' in "readiness"

[dvyukov, 2013-03-21 08:54:03]

On 2013/03/18 20:56:02, bradfitz wrote:
> LGTM
> 
> https://codereview.appspot.com/7870043/diff/12001/src/pkg/net/fd_unix.go
> File src/pkg/net/fd_unix.go (right):
> 
>
https://codereview.appspot.com/7870043/diff/12001/src/pkg/net/fd_unix.go#newc...
> src/pkg/net/fd_unix.go:127: // Poller may want to unregister fd in rediness
> notification mechanism,
> missing an 'a' in "readiness"

Done

[dvyukov, 2013-03-21 08:54:25]

*** Submitted as https://code.google.com/p/go/source/detail?r=ff779e477085 ***

runtime: explicitly remove fd's from epoll waitset before close()
Fixes issue 5061.

Current code relies on the fact that fd's are automatically removed from epoll
set when closed. However, it is not true. Underlying file description is removed
from epoll set only when *all* fd's referring to it are closed.

There are 2 bad consequences:
1. Kernel delivers notifications on already closed fd's.
2. The following sequence of events leads to error:
   - add fd1 to epoll
   - dup fd1 = fd2
   - close fd1 (not removed from epoll since we've dup'ed the fd)
   - dup fd2 = fd1 (get the same fd as fd1)
   - add fd1 to epoll = EEXIST

So, if fd can be potentially dup'ed of fork'ed, it's necessary to explicitly
remove the fd from epoll set.

R=golang-dev, bradfitz, dave
CC=golang-dev
https://codereview.appspot.com/7870043