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

Oleg Nesterov
2009-06-28T21:19:06Z

On 06/28, Jarek Poplawski wrote:
>
> Jarek Poplawski wrote, On 06/28/2009 01:10 PM:
>
> > Oleg Nesterov wrote, On 06/26/2009 04:50 PM:
> >
> >> Perhaps we can add
> >>
> >> void sock_poll_wait(struct file *file, struct sock *sk, poll_table *pt)
> >> {
> >> if (pt) {
> >> poll_wait(file, sk->sk_sleep, pt);
> >> /*
> >> * fat comment
> >> */
> >> smp_mb(); // or smp_mb__after_unlock();
> >> }
> >> }
> >>
> >> Oleg.
> >
> >
> > Maybe 'a bit' further?:
> >
> > static inline void __poll_wait(struct file * filp, wait_queue_head_t * wait_address, poll_table *p)
> > {
> > p->qproc(filp, wait_address, p);
> > }
> >
> > static inline void poll_wait(struct file * filp, wait_queue_head_t * wait_address, poll_table *p)
> > {
> > if (p && wait_address)
> > __poll_wait(filp, wait_address, p);
> > }
> >
> > static inline void sock_poll_wait(struct file * filp, wait_queue_head_t * wait_address, poll_table *p)
> > {
> > if (p && wait_address) {
> > __poll_wait(filp, wait_address, p);
> > /*
> > * fat comment
> > */
> > smp_mb(); // or smp_mb__after_unlock();
> > }
> > }
> >
>
>
> Hmm... of course:
>
> static inline void sock_poll_wait(struct file * filp, struct sock *sk, poll_table *p)
> {
> if (p && sk->sk_sleep) {
> __poll_wait(filp, sk->sk_sleep, p);
> /*
> * fat comment
> */
> smp_mb(); // or smp_mb__after_unlock();
> }
> }

Perhaps it makes sense to check ->sk_sleep != NULL in sock_poll_wait(), but
I don't think we need __poll_wait(). poll_wait() is inline, I think gcc
will optimize out "if (p && wait_address)" check if poll_wait() is called
from sock_poll_wait().

This all is up to Jiri of course. But speaking about cosmetic changes, I
think it is better to make 2 patches. The first one fixes the problem using
smp_mb(), another introduces smp_mb__xxx_lock() to optimize the code.

Oleg.