Re: [PATCH] net: fix race in the receive/select

Jarek Poplawski
2009-06-26T08:10:41Z

On Fri, Jun 26, 2009 at 05:42:30AM +0000, Jarek Poplawski wrote:
> On 26-06-2009 05:14, Davide Libenzi wrote:
> > On Fri, 26 Jun 2009, Eric Dumazet wrote:
> >
> >> I wont argue with you David, just try to correct bugs.
> >>
> >> fs/ext4/ioctl.c line 182
> >>
> >> set_current_state(TASK_INTERRUPTIBLE);
> >> add_wait_queue(&EXT4_SB(sb)->ro_wait_queue, &wait);
> >> if (timer_pending(&EXT4_SB(sb)->turn_ro_timer)) {
> >> schedule();
> >>
> >> Another example of missing barrier after add_wait_queue()
> >>
> >> Because add_wait_queue() misses a barrier, we have to add one after each call.
> >>
> >> Maybe it would be safer to add barrier in add_wait_queue() itself, not in _pollwait().
> >
> > Not all the code that uses add_wait_queue() does need to have the MB,
> > like code that does the most common pattern:
> >
> > xxx_poll(...) {
> > poll_wait(...);
> > lock();
> > flags = calc_flags(->status);
> > unlock();
> > return flags;
> > }
> >
> > xxx_update(...) {
> > lock();
> > ->status = ...;
> > unlock();
> > if (waitqueue_active())
> > wake_up();
> > }
> >
> > It's the code that does the lockless flags calculation in ->poll that
> > might need it.
> > I dunno what the amount of changes are, but cross-matching MB across
> > subsystems does not look nice.
> > IMHO that's a detail of the subsystem locking, and should be confined
> > inside the subsystem itself.
> > No?
>
> How about poll_wait_mb() and waitqueue_active_mb() (with mb and
> additional check for NULL of wait_queue_head)?

Hmm... But considering Eric's arguments I see it would be hard
/impossible to do it with the current api, so let's forget.

Jarek P.