Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1276511 > unrolled thread

use-after-free in sock_wake_async

Back to article view | Back to linux.kernel

Contents

  use-after-free in sock_wake_async Dmitry Vyukov <dvyukov@google.com> - 2015-11-24 15:20 +0100
    Re: use-after-free in sock_wake_async Eric Dumazet <edumazet@google.com> - 2015-11-24 16:30 +0100
      Re: use-after-free in sock_wake_async Eric Dumazet <eric.dumazet@gmail.com> - 2015-11-24 16:40 +0100
      Re: use-after-free in sock_wake_async Jason Baron <jbaron@akamai.com> - 2015-11-24 22:40 +0100
        Re: use-after-free in sock_wake_async Benjamin LaHaise <bcrl@kvack.org> - 2015-11-24 22:50 +0100
          Re: use-after-free in sock_wake_async Eric Dumazet <edumazet@google.com> - 2015-11-24 23:10 +0100
            Re: use-after-free in sock_wake_async Eric Dumazet <edumazet@google.com> - 2015-11-24 23:20 +0100
        Re: use-after-free in sock_wake_async Al Viro <viro@ZenIV.linux.org.uk> - 2015-11-24 22:50 +0100
      Re: use-after-free in sock_wake_async Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2015-11-25 00:40 +0100
        Re: use-after-free in sock_wake_async Eric Dumazet <edumazet@google.com> - 2015-11-25 00:50 +0100
          Re: use-after-free in sock_wake_async Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2015-11-25 02:20 +0100
          Re: use-after-free in sock_wake_async Eric Dumazet <edumazet@google.com> - 2015-11-25 02:20 +0100
            Re: use-after-free in sock_wake_async Eric Dumazet <eric.dumazet@gmail.com> - 2015-11-25 03:30 +0100
              Re: use-after-free in sock_wake_async Eric Dumazet <eric.dumazet@gmail.com> - 2015-11-25 06:50 +0100
                Re: use-after-free in sock_wake_async Eric Dumazet <eric.dumazet@gmail.com> - 2015-11-25 15:20 +0100
            Re: use-after-free in sock_wake_async Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2015-11-25 17:50 +0100
              Re: use-after-free in sock_wake_async Eric Dumazet <eric.dumazet@gmail.com> - 2015-11-25 18:20 +0100
                Re: use-after-free in sock_wake_async Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2015-11-25 18:40 +0100
                  Re: use-after-free in sock_wake_async Eric Dumazet <eric.dumazet@gmail.com> - 2015-11-25 19:00 +0100
                    Re: use-after-free in sock_wake_async Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2015-11-25 19:30 +0100
                      Re: use-after-free in sock_wake_async Eric Dumazet <eric.dumazet@gmail.com> - 2015-11-25 19:40 +0100
                        Re: use-after-free in sock_wake_async Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2015-11-25 20:40 +0100
                          Re: use-after-free in sock_wake_async Eric Dumazet <eric.dumazet@gmail.com> - 2015-11-25 21:00 +0100
                            Re: use-after-free in sock_wake_async Eric Dumazet <eric.dumazet@gmail.com> - 2015-11-25 21:30 +0100
                              Re: use-after-free in sock_wake_async Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2015-11-25 22:00 +0100
                                Re: use-after-free in sock_wake_async Eric Dumazet <eric.dumazet@gmail.com> - 2015-11-25 23:10 +0100
                                  Re: use-after-free in sock_wake_async Hannes Frederic Sowa <hannes@stressinduktion.org> - 2015-11-25 23:40 +0100
                                    Re: use-after-free in sock_wake_async Eric Dumazet <eric.dumazet@gmail.com> - 2015-11-25 23:50 +0100
                                      Re: use-after-free in sock_wake_async Hannes Frederic Sowa <hannes@stressinduktion.org> - 2015-11-26 00:00 +0100
                                        Re: use-after-free in sock_wake_async Hannes Frederic Sowa <hannes@stressinduktion.org> - 2015-11-26 14:40 +0100
                                          Re: use-after-free in sock_wake_async Hannes Frederic Sowa <hannes@stressinduktion.org> - 2015-11-26 15:40 +0100
                                          Re: use-after-free in sock_wake_async Eric Dumazet <eric.dumazet@gmail.com> - 2015-11-26 17:00 +0100
                                            Re: use-after-free in sock_wake_async Eric Dumazet <eric.dumazet@gmail.com> - 2015-11-26 18:10 +0100
                                              Re: use-after-free in sock_wake_async Hannes Frederic Sowa <hannes@stressinduktion.org> - 2015-11-26 18:20 +0100
                                            Re: use-after-free in sock_wake_async Hannes Frederic Sowa <hannes@stressinduktion.org> - 2015-11-26 18:10 +0100
                                              Re: use-after-free in sock_wake_async Eric Dumazet <eric.dumazet@gmail.com> - 2015-11-26 18:30 +0100
          Re: use-after-free in sock_wake_async Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2015-11-25 02:20 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1277693

On Wed, 2015-11-25 at 18:24 +0000, Rainer Weikusat wrote:
> Eric Dumazet <eric.dumazet@gmail.com> writes:
> > On Wed, 2015-11-25 at 17:30 +0000, Rainer Weikusat wrote:
> >
> >> In case this is wrong, it obviously implies that sk_sleep(sk) must not
> >> be used anywhere as it accesses the same struck sock, hence, when that
> >> can "suddenly" disappear despite locks are used in the way indicated
> >> above, there is now safe way to invoke that, either, as it just does a
> >> rcu_dereference_raw based on the assumption that the caller knows that
> >> the i-node (and the corresponding wait queue) still exist.
> >> 
> >
> > Oh well.
> >
> > sk_sleep() is not used if the return is NULL
> 
> static long unix_stream_data_wait(struct sock *sk, long timeo,
> 				  struct sk_buff *last, unsigned int last_len)
> {
> 	struct sk_buff *tail;
> 	DEFINE_WAIT(wait);
> 
> 	unix_state_lock(sk);
> 
> 	for (;;) {
> 		prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
> 
> 		tail = skb_peek_tail(&sk->sk_receive_queue);
> 		if (tail != last ||
> 		    (tail && tail->len != last_len) ||
> 		    sk->sk_err ||
> 		    (sk->sk_shutdown & RCV_SHUTDOWN) ||
> 		    signal_pending(current) ||
> 		    !timeo)
> 			break;
> 
> 		set_bit(SOCK_ASYNC_WAITDATA, &sk->sk_socket->flags);
> 		unix_state_unlock(sk);
> 		timeo = freezable_schedule_timeout(timeo);
> 		unix_state_lock(sk);
> 
> 		if (sock_flag(sk, SOCK_DEAD))
> 			break;
> 
> 		clear_bit(SOCK_ASYNC_WAITDATA, &sk->sk_socket->flags);
> 	}
> 
> 	finish_wait(sk_sleep(sk), &wait);
> 	unix_state_unlock(sk);
> 	return timeo;
> }
> 
> Neither prepare_to_wait nor finish_wait check if the pointer is
> null. For the finish_wait case, it shouldn't be null because if
> SOCK_DEAD is not found to be set after the unix_state_lock was acquired,
> unix_release_sock didn't execute the corresponding code yet, hence,
> inode etc will remain available until after the corresponding unlock.


> 
> But this isn't true anymore if the inode can go away despite
> sock_release couldn't complete yet.


You are looking at the wrong side.

Of course, the thread 'owning' a socket has a reference on it, so it
knows sk->sk_socket and sk->sk_ww is not NULL.

The problem is that at the time a wakeup is done, it can be done by a
process or softirq having no ref on the 'struct socket', as
sk->sk_socket can become NULL at anytime.

This is why we have sk_wq , and RCU protection, so that we do not have
to use expensive atomic operations in this fast path.




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277733

Eric Dumazet <eric.dumazet@gmail.com> writes:
> On Wed, 2015-11-25 at 18:24 +0000, Rainer Weikusat wrote:
>> Eric Dumazet <eric.dumazet@gmail.com> writes:
>> > On Wed, 2015-11-25 at 17:30 +0000, Rainer Weikusat wrote:
>> >
>> >> In case this is wrong, it obviously implies that sk_sleep(sk) must not
>> >> be used anywhere as it accesses the same struck sock, hence, when that
>> >> can "suddenly" disappear despite locks are used in the way indicated
>> >> above, there is now safe way to invoke that, either, as it just does a
>> >> rcu_dereference_raw based on the assumption that the caller knows that
>> >> the i-node (and the corresponding wait queue) still exist.
>> >> 
>> >
>> > Oh well.
>> >
>> > sk_sleep() is not used if the return is NULL

[...]

>> 	finish_wait(sk_sleep(sk), &wait);
>> 	unix_state_unlock(sk);
>> 	return timeo;
>> }
>> 
>> Neither prepare_to_wait nor finish_wait check if the pointer is
>> null.

[...]

> You are looking at the wrong side.
>
> Of course, the thread 'owning' a socket has a reference on it, so it
> knows sk->sk_socket and sk->sk_ww is not NULL.

I could continue argueing about this but IMHO, it just leads away from
the actual issue. Taking a couple of steps back, therefore,

------------
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 4e95bdf..5c87ea6 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1754,8 +1754,8 @@ restart_locked:
        skb_queue_tail(&other->sk_receive_queue, skb);
        if (max_level > unix_sk(other)->recursion_level)
                unix_sk(other)->recursion_level = max_level;
-       unix_state_unlock(other);
        other->sk_data_ready(other);
+       unix_state_unlock(other);
        sock_put(other);
        scm_destroy(&scm);
        return len;
@@ -1860,8 +1860,8 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
                skb_queue_tail(&other->sk_receive_queue, skb);
                if (max_level > unix_sk(other)->recursion_level)
                        unix_sk(other)->recursion_level = max_level;
-               unix_state_unlock(other);
                other->sk_data_ready(other);
+               unix_state_unlock(other);
                sent += size;
        }
 
-------------

I'm convinced this will work for the given problem (I don't claim that
it's technically superior to the larger change in any aspect except that
it's simpler and localized) because

1) The use-after-free occurred while accessing the struck sock allocated
   together with the socket inode.

2) This happened because a close was racing with a write.   

3) The socket inode, struct sock and struct socket_wq are freed by
   sock_destroy_inode.

4) sock_destroy_inode can only be called as consequence of the iput in
   sock_release.

5) sock_release invokes the per-protocol/ family release function before
   doing the iput.

6) unix_sock_release has to acquire the unix_state_lock on the socket
   referred to as other in the code above before it can do anything, in
   particular, before it calls sock_orphan which resets the struct sock and wq
   pointers and also sets the SOCK_DEAD flag.

7) the unix_stream_sendmsg code acquires the unix_state_lock on other
   and then checks the SOCK_DEAD flag. The code above only runs if it
   was not set, hence, the iput in sock_release can't have happened yet
   because a concurrent unix_sock_release must still be blocked on the
   unix_state_lock of other.

If there's an error in this reasoning, I'd very much like to know where
and what it is, not the least because the unix_dgram_peer_wake_relay
function I wrote also relies on it being correct (wrt to using the
result of sk_sleep outside of rcu_read_lock/ rcu_read_unlock).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277759

On Wed, 2015-11-25 at 19:38 +0000, Rainer Weikusat wrote:
> Eric Dumazet <eric.dumazet@gmail.com> writes:
> > On Wed, 2015-11-25 at 18:24 +0000, Rainer Weikusat wrote:
> >> Eric Dumazet <eric.dumazet@gmail.com> writes:
> >> > On Wed, 2015-11-25 at 17:30 +0000, Rainer Weikusat wrote:
> >> >
> >> >> In case this is wrong, it obviously implies that sk_sleep(sk) must not
> >> >> be used anywhere as it accesses the same struck sock, hence, when that
> >> >> can "suddenly" disappear despite locks are used in the way indicated
> >> >> above, there is now safe way to invoke that, either, as it just does a
> >> >> rcu_dereference_raw based on the assumption that the caller knows that
> >> >> the i-node (and the corresponding wait queue) still exist.
> >> >> 
> >> >
> >> > Oh well.
> >> >
> >> > sk_sleep() is not used if the return is NULL
> 
> [...]
> 
> >> 	finish_wait(sk_sleep(sk), &wait);
> >> 	unix_state_unlock(sk);
> >> 	return timeo;
> >> }
> >> 
> >> Neither prepare_to_wait nor finish_wait check if the pointer is
> >> null.
> 
> [...]
> 
> > You are looking at the wrong side.
> >
> > Of course, the thread 'owning' a socket has a reference on it, so it
> > knows sk->sk_socket and sk->sk_ww is not NULL.
> 
> I could continue argueing about this but IMHO, it just leads away from
> the actual issue. Taking a couple of steps back, therefore,
> 
> ------------
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 4e95bdf..5c87ea6 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1754,8 +1754,8 @@ restart_locked:
>         skb_queue_tail(&other->sk_receive_queue, skb);
>         if (max_level > unix_sk(other)->recursion_level)
>                 unix_sk(other)->recursion_level = max_level;
> -       unix_state_unlock(other);
>         other->sk_data_ready(other);
> +       unix_state_unlock(other);
>         sock_put(other);
>         scm_destroy(&scm);
>         return len;
> @@ -1860,8 +1860,8 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
>                 skb_queue_tail(&other->sk_receive_queue, skb);
>                 if (max_level > unix_sk(other)->recursion_level)
>                         unix_sk(other)->recursion_level = max_level;
> -               unix_state_unlock(other);
>                 other->sk_data_ready(other);
> +               unix_state_unlock(other);
>                 sent += size;
>         }
>  
> -------------
> 
> I'm convinced this will work for the given problem (I don't claim that
> it's technically superior to the larger change in any aspect except that
> it's simpler and localized) because
> 
> 1) The use-after-free occurred while accessing the struck sock allocated
>    together with the socket inode.
> 
> 2) This happened because a close was racing with a write.   
> 
> 3) The socket inode, struct sock and struct socket_wq are freed by
>    sock_destroy_inode.
> 
> 4) sock_destroy_inode can only be called as consequence of the iput in
>    sock_release.
> 
> 5) sock_release invokes the per-protocol/ family release function before
>    doing the iput.
> 
> 6) unix_sock_release has to acquire the unix_state_lock on the socket
>    referred to as other in the code above before it can do anything, in
>    particular, before it calls sock_orphan which resets the struct sock and wq
>    pointers and also sets the SOCK_DEAD flag.
> 
> 7) the unix_stream_sendmsg code acquires the unix_state_lock on other
>    and then checks the SOCK_DEAD flag. The code above only runs if it
>    was not set, hence, the iput in sock_release can't have happened yet
>    because a concurrent unix_sock_release must still be blocked on the
>    unix_state_lock of other.
> 
> If there's an error in this reasoning, I'd very much like to know where
> and what it is, not the least because the unix_dgram_peer_wake_relay
> function I wrote also relies on it being correct (wrt to using the
> result of sk_sleep outside of rcu_read_lock/ rcu_read_unlock).

Yeah, we can either continue to patch particular buggy code, with hard
to review and debug stuff (this particular issue is not new)

OR, we add core infrastructure and we have less headaches, because it
works for all sockets, including af_unix.

My choice is pretty clear, having to maintain this code at Google, I can
tell you that solid core networking stack is much much better by all
means.

This af_unix code needs serious rewrite, we have absolute nightmares
with it (like the bugs added in recent sendpage() support).

I believe anything we can do to avoid having to make sure the points you
listed are going to be kept at next patch or code refactoring is a big
win.




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277777

On Wed, 2015-11-25 at 11:50 -0800, Eric Dumazet wrote:

> >                 other->sk_data_ready(other);
> > +               unix_state_unlock(other);


Also, problem with such construct is that we wakeup a thread that will
block on the lock we hold.

Beauty of sk_data_ready() is to call it once we hold no lock any more,
to enable another cpu to immediately proceed.

In this case, 'other' can not disappear, so it should be safe.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277794

Eric Dumazet <eric.dumazet@gmail.com> writes:
> On Wed, 2015-11-25 at 11:50 -0800, Eric Dumazet wrote:
>
>> >                 other->sk_data_ready(other);
>> > +               unix_state_unlock(other);
>
>
> Also, problem with such construct is that we wakeup a thread that will
> block on the lock we hold.
>
> Beauty of sk_data_ready() is to call it once we hold no lock any more,
> to enable another cpu to immediately proceed.
>
> In this case, 'other' can not disappear, so it should be safe.

I do agree that keeping the ->sk_data_ready outside of the lock will
very likely have performance advantages. That's just something I
wouldn't have undertaken because I'd be reluctant to make a fairly
complicated change to a lot of code in order to improve performance
unless performance was actually found to be lacking and because it would
step onto to many different people's turf.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277842

On Wed, 2015-11-25 at 20:57 +0000, Rainer Weikusat wrote:

> I do agree that keeping the ->sk_data_ready outside of the lock will
> very likely have performance advantages. That's just something I
> wouldn't have undertaken because I'd be reluctant to make a fairly
> complicated change to a lot of code.

All I am saying is that we can keep current performance.

We already have the core infrastructure, we only need to properly use
it.

I will split my changes in two parts.

One part doing a very boring change of

rename SOCK_ASYNC_NOSPACE and SOCK_ASYNC_WAITDATA
for X in SOCK_ASYNC_NOSPACE SOCK_ASYNC_WAITDATA 

 set_bit(X, &sk->sk_socket->flags) -> sk_set_bit(X, sk)
 clear_bit(X, &sk->sk_socket->flags) -> sk_clear_bit(X, sk)

The rename will help backports to catch code that might have been
removed in recent kernels.

Then the second patch will do the actual changes, and they will look
very sensible for people wanting to review them, and or familiar with
the stack, do not worry ;)



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277852

On Wed, Nov 25, 2015, at 23:09, Eric Dumazet wrote:
> On Wed, 2015-11-25 at 20:57 +0000, Rainer Weikusat wrote:
> 
> > I do agree that keeping the ->sk_data_ready outside of the lock will
> > very likely have performance advantages. That's just something I
> > wouldn't have undertaken because I'd be reluctant to make a fairly
> > complicated change to a lot of code.
> 
> All I am saying is that we can keep current performance.
> 
> We already have the core infrastructure, we only need to properly use
> it.
> 
> I will split my changes in two parts.
> 
> One part doing a very boring change of
> 
> rename SOCK_ASYNC_NOSPACE and SOCK_ASYNC_WAITDATA
> for X in SOCK_ASYNC_NOSPACE SOCK_ASYNC_WAITDATA 
> 
>  set_bit(X, &sk->sk_socket->flags) -> sk_set_bit(X, sk)
>  clear_bit(X, &sk->sk_socket->flags) -> sk_clear_bit(X, sk)

sk_set_bit and sk_clear_bit will forward the set_bit and clear_bit into
the socket_wq like you explained above?

> The rename will help backports to catch code that might have been
> removed in recent kernels.
> 
> Then the second patch will do the actual changes, and they will look
> very sensible for people wanting to review them, and or familiar with
> the stack, do not worry ;)

Do you see a chance to inline socket_wq into struct socket and discard
struct socket_alloc in one go by rcu in socket_destroy_inode?

Thanks,
Hannes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277857

On Wed, 2015-11-25 at 23:32 +0100, Hannes Frederic Sowa wrote:
> On Wed, Nov 25, 2015, at 23:09, Eric Dumazet wrote:
> > On Wed, 2015-11-25 at 20:57 +0000, Rainer Weikusat wrote:
> > 
> > > I do agree that keeping the ->sk_data_ready outside of the lock will
> > > very likely have performance advantages. That's just something I
> > > wouldn't have undertaken because I'd be reluctant to make a fairly
> > > complicated change to a lot of code.
> > 
> > All I am saying is that we can keep current performance.
> > 
> > We already have the core infrastructure, we only need to properly use
> > it.
> > 
> > I will split my changes in two parts.
> > 
> > One part doing a very boring change of
> > 
> > rename SOCK_ASYNC_NOSPACE and SOCK_ASYNC_WAITDATA
> > for X in SOCK_ASYNC_NOSPACE SOCK_ASYNC_WAITDATA 
> > 
> >  set_bit(X, &sk->sk_socket->flags) -> sk_set_bit(X, sk)
> >  clear_bit(X, &sk->sk_socket->flags) -> sk_clear_bit(X, sk)
> 
> sk_set_bit and sk_clear_bit will forward the set_bit and clear_bit into
> the socket_wq like you explained above?

In the first patch (no functional change), the helpers will look like

static void inline sk_set_bit(int nr, struct sock *sk)
{
	set_bit(nr, &sk->sk_socket->flags);
}


Then the second patch will change the helper to :

static void inline sk_set_bit(int nr, struct sock *sk)
{
	set_bit(nr, &sk->sk_wq_raw->flags);
}


> 
> > The rename will help backports to catch code that might have been
> > removed in recent kernels.
> > 
> > Then the second patch will do the actual changes, and they will look
> > very sensible for people wanting to review them, and or familiar with
> > the stack, do not worry ;)
> 
> Do you see a chance to inline socket_wq into struct socket and discard
> struct socket_alloc in one go by rcu in socket_destroy_inode?

???

I guess you missed the whole point to have socket_wq allocated outside
of the inode :(

inodes are not rcu protected (yet). I certainly don't want to mess with
VFS, we have enough problems in net/ directory already.




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277862

On Wed, Nov 25, 2015, at 23:43, Eric Dumazet wrote:
> On Wed, 2015-11-25 at 23:32 +0100, Hannes Frederic Sowa wrote:
> > On Wed, Nov 25, 2015, at 23:09, Eric Dumazet wrote:
> > > On Wed, 2015-11-25 at 20:57 +0000, Rainer Weikusat wrote:
> > > 
> > > > I do agree that keeping the ->sk_data_ready outside of the lock will
> > > > very likely have performance advantages. That's just something I
> > > > wouldn't have undertaken because I'd be reluctant to make a fairly
> > > > complicated change to a lot of code.
> > > 
> > > All I am saying is that we can keep current performance.
> > > 
> > > We already have the core infrastructure, we only need to properly use
> > > it.
> > > 
> > > I will split my changes in two parts.
> > > 
> > > One part doing a very boring change of
> > > 
> > > rename SOCK_ASYNC_NOSPACE and SOCK_ASYNC_WAITDATA
> > > for X in SOCK_ASYNC_NOSPACE SOCK_ASYNC_WAITDATA 
> > > 
> > >  set_bit(X, &sk->sk_socket->flags) -> sk_set_bit(X, sk)
> > >  clear_bit(X, &sk->sk_socket->flags) -> sk_clear_bit(X, sk)
> > 
> > sk_set_bit and sk_clear_bit will forward the set_bit and clear_bit into
> > the socket_wq like you explained above?
> 
> In the first patch (no functional change), the helpers will look like
> 
> static void inline sk_set_bit(int nr, struct sock *sk)
> {
> 	set_bit(nr, &sk->sk_socket->flags);
> }
> 
> 
> Then the second patch will change the helper to :
> 
> static void inline sk_set_bit(int nr, struct sock *sk)
> {
> 	set_bit(nr, &sk->sk_wq_raw->flags);
> }

Yep, that looks sensible.

> > > The rename will help backports to catch code that might have been
> > > removed in recent kernels.
> > > 
> > > Then the second patch will do the actual changes, and they will look
> > > very sensible for people wanting to review them, and or familiar with
> > > the stack, do not worry ;)
> > 
> > Do you see a chance to inline socket_wq into struct socket and discard
> > struct socket_alloc in one go by rcu in socket_destroy_inode?
> 
> ???
> 
> I guess you missed the whole point to have socket_wq allocated outside
> of the inode :(

Yep, sure, so inode could be torn down while wq is freed by rcu
callback.

> inodes are not rcu protected (yet). I certainly don't want to mess with
> VFS, we have enough problems in net/ directory already.

I have seen filesystems already doing so in .destroy_inode, that's why I
am asking. The allocation happens the same way as we do with sock_alloc,
e.g. shmem. I actually thought that struct inode already provides an
rcu_head for exactly that reason.

Bye,
Hannes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278217

Hannes Frederic Sowa <hannes@stressinduktion.org> writes:


> I have seen filesystems already doing so in .destroy_inode, that's why I
> am asking. The allocation happens the same way as we do with sock_alloc,
> e.g. shmem. I actually thought that struct inode already provides an
> rcu_head for exactly that reason.

E.g.:

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 54036ae..e15c49f 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -35,7 +35,6 @@
 struct macvtap_queue {
 	struct sock sk;
 	struct socket sock;
-	struct socket_wq wq;
 	int vnet_hdr_sz;
 	struct macvlan_dev __rcu *vlan;
 	struct file *file;
@@ -529,8 +528,7 @@ static int macvtap_open(struct inode *inode, struct file *file)
 	if (!q)
 		goto out;
 
-	RCU_INIT_POINTER(q->sock.wq, &q->wq);
-	init_waitqueue_head(&q->wq.wait);
+	init_waitqueue_head(&q->sock.wq.wait);
 	q->sock.type = SOCK_RAW;
 	q->sock.state = SS_CONNECTED;
 	q->sock.file = file;
@@ -579,7 +577,7 @@ static unsigned int macvtap_poll(struct file *file, poll_table * wait)
 		goto out;
 
 	mask = 0;
-	poll_wait(file, &q->wq.wait, wait);
+	poll_wait(file, &q->sock.wq.wait, wait);
 
 	if (!skb_queue_empty(&q->sk.sk_receive_queue))
 		mask |= POLLIN | POLLRDNORM;
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index b1878fa..20c5d34 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -145,7 +145,6 @@ struct tap_filter {
 struct tun_file {
 	struct sock sk;
 	struct socket socket;
-	struct socket_wq wq;
 	struct tun_struct __rcu *tun;
 	struct fasync_struct *fasync;
 	/* only used for fasnyc */
@@ -2219,8 +2218,7 @@ static int tun_chr_open(struct inode *inode, struct file * file)
 	tfile->flags = 0;
 	tfile->ifindex = 0;
 
-	init_waitqueue_head(&tfile->wq.wait);
-	RCU_INIT_POINTER(tfile->socket.wq, &tfile->wq);
+	init_waitqueue_head(&tfile->socket.wq.wait);
 
 	tfile->socket.file = file;
 	tfile->socket.ops = &tun_socket_ops;
diff --git a/include/linux/net.h b/include/linux/net.h
index 70ac5e2..3a7a4d1 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -89,8 +89,7 @@ struct socket_wq {
 	/* Note: wait MUST be first field of socket_wq */
 	wait_queue_head_t	wait;
 	struct fasync_struct	*fasync_list;
-	struct rcu_head		rcu;
-} ____cacheline_aligned_in_smp;
+};
 
 /**
  *  struct socket - general BSD socket
@@ -111,7 +110,7 @@ struct socket {
 
 	unsigned long		flags;
 
-	struct socket_wq __rcu	*wq;
+	struct socket_wq	wq;
 
 	struct file		*file;
 	struct sock		*sk;
diff --git a/include/net/sock.h b/include/net/sock.h
index 7f89e4b..ae34da1 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1674,7 +1674,7 @@ static inline void sock_orphan(struct sock *sk)
 static inline void sock_graft(struct sock *sk, struct socket *parent)
 {
 	write_lock_bh(&sk->sk_callback_lock);
-	sk->sk_wq = parent->wq;
+	sk->sk_wq = &parent->wq;
 	parent->sk = sk;
 	sk_set_socket(sk, parent);
 	security_sock_graft(sk, parent);
diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 630c197..c125881 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -657,7 +657,7 @@ static void rcu_preempt_do_callbacks(void)
 /*
  * Queue a preemptible-RCU callback for invocation after a grace period.
  */
-void call_rcu(struct rcu_head *head, rcu_callback_t func)
+static void call_rcu(struct rcu_head *head, rcu_callback_t func)
 {
 	__call_rcu(head, func, rcu_state_p, -1, 0);
 }
diff --git a/net/core/sock.c b/net/core/sock.c
index 1e4dd54..314ab6a 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2383,7 +2383,7 @@ void sock_init_data(struct socket *sock, struct sock *sk)
 
 	if (sock) {
 		sk->sk_type	=	sock->type;
-		sk->sk_wq	=	sock->wq;
+		sk->sk_wq	=	&sock->wq;
 		sock->sk	=	sk;
 	} else
 		sk->sk_wq	=	NULL;
diff --git a/net/socket.c b/net/socket.c
index dd2c247..495485e 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -245,19 +245,12 @@ static struct kmem_cache *sock_inode_cachep __read_mostly;
 static struct inode *sock_alloc_inode(struct super_block *sb)
 {
 	struct socket_alloc *ei;
-	struct socket_wq *wq;
 
 	ei = kmem_cache_alloc(sock_inode_cachep, GFP_KERNEL);
 	if (!ei)
 		return NULL;
-	wq = kmalloc(sizeof(*wq), GFP_KERNEL);
-	if (!wq) {
-		kmem_cache_free(sock_inode_cachep, ei);
-		return NULL;
-	}
-	init_waitqueue_head(&wq->wait);
-	wq->fasync_list = NULL;
-	RCU_INIT_POINTER(ei->socket.wq, wq);
+	init_waitqueue_head(&ei->socket.wq.wait);
+	ei->socket.wq.fasync_list = NULL;
 
 	ei->socket.state = SS_UNCONNECTED;
 	ei->socket.flags = 0;
@@ -268,17 +261,18 @@ static struct inode *sock_alloc_inode(struct super_block *sb)
 	return &ei->vfs_inode;
 }
 
-static void sock_destroy_inode(struct inode *inode)
+static void sock_cache_free_rcu(struct rcu_head *rcu)
 {
-	struct socket_alloc *ei;
-	struct socket_wq *wq;
-
-	ei = container_of(inode, struct socket_alloc, vfs_inode);
-	wq = rcu_dereference_protected(ei->socket.wq, 1);
-	kfree_rcu(wq, rcu);
+	struct socket_alloc *ei =
+		container_of(rcu, struct socket_alloc, vfs_inode.i_rcu);
 	kmem_cache_free(sock_inode_cachep, ei);
 }
 
+static void sock_destroy_inode(struct inode *inode)
+{
+	call_rcu(&inode->i_rcu, sock_cache_free_rcu);
+}
+
 static void init_once(void *foo)
 {
 	struct socket_alloc *ei = (struct socket_alloc *)foo;
@@ -573,7 +567,7 @@ void sock_release(struct socket *sock)
 		module_put(owner);
 	}
 
-	if (rcu_dereference_protected(sock->wq, 1)->fasync_list)
+	if (sock->wq.fasync_list)
 		pr_err("%s: fasync list not empty!\n", __func__);
 
 	this_cpu_sub(sockets_in_use, 1);
@@ -1044,7 +1038,7 @@ static int sock_fasync(int fd, struct file *filp, int on)
 		return -EINVAL;
 
 	lock_sock(sk);
-	wq = rcu_dereference_protected(sock->wq, sock_owned_by_user(sk));
+	wq = &sock->wq;
 	fasync_helper(fd, filp, on, &wq->fasync_list);
 
 	if (!wq->fasync_list)
@@ -1065,7 +1059,7 @@ int sock_wake_async(struct socket *sock, int how, int band)
 	if (!sock)
 		return -1;
 	rcu_read_lock();
-	wq = rcu_dereference(sock->wq);
+	wq = &sock->wq;
 	if (!wq || !wq->fasync_list) {
 		rcu_read_unlock();
 		return -1;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278262

On Thu, Nov 26, 2015, at 14:32, Hannes Frederic Sowa wrote:
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 7f89e4b..ae34da1 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1674,7 +1674,7 @@ static inline void sock_orphan(struct sock *sk)
>  static inline void sock_graft(struct sock *sk, struct socket *parent)
>  {
>  	write_lock_bh(&sk->sk_callback_lock);
> -       sk->sk_wq = parent->wq;
> +       sk->sk_wq = &parent->wq;

RCU_INIT_POINTER(sk->sk_wq, &parent->wq);

>  	parent->sk = sk;
>  	sk_set_socket(sk, parent);
>  	security_sock_graft(sk, parent);
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index 630c197..c125881 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -657,7 +657,7 @@ static void rcu_preempt_do_callbacks(void)
>  /*
>   * Queue a preemptible-RCU callback for invocation after a grace period.
>   */
> -void call_rcu(struct rcu_head *head, rcu_callback_t func)
> +static void call_rcu(struct rcu_head *head, rcu_callback_t func)
>  {
>  	__call_rcu(head, func, rcu_state_p, -1, 0);
>  }
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 1e4dd54..314ab6a 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2383,7 +2383,7 @@ void sock_init_data(struct socket *sock, struct
> sock *sk)
>  
>  	if (sock) {
>  		sk->sk_type	=	sock->type;
> -               sk->sk_wq       =       sock->wq;
> +               sk->sk_wq       =       &sock->wq;

RCU_INIT_POINTER()

>  		sock->sk	=	sk;
>  	} else
>  		sk->sk_wq	=	NULL;
> diff --git a/net/socket.c b/net/socket.c
> index dd2c247..495485e 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -245,19 +245,12 @@ static struct kmem_cache *sock_inode_cachep
> __read_mostly;
>  static struct inode *sock_alloc_inode(struct super_block *sb)
>  {
>  	struct socket_alloc *ei;
> -       struct socket_wq *wq;
>  
>  	ei = kmem_cache_alloc(sock_inode_cachep, GFP_KERNEL);
>  	if (!ei)
>  		return NULL;
> -       wq = kmalloc(sizeof(*wq), GFP_KERNEL);
> -       if (!wq) {
> -               kmem_cache_free(sock_inode_cachep, ei);
> -               return NULL;
> -       }
> -       init_waitqueue_head(&wq->wait);
> -       wq->fasync_list = NULL;
> -       RCU_INIT_POINTER(ei->socket.wq, wq);
> +       init_waitqueue_head(&ei->socket.wq.wait);
> +       ei->socket.wq.fasync_list = NULL;
>  
>  	ei->socket.state = SS_UNCONNECTED;
>  	ei->socket.flags = 0;
> @@ -268,17 +261,18 @@ static struct inode *sock_alloc_inode(struct
> super_block *sb)
>  	return &ei->vfs_inode;
>  }
>  
> -static void sock_destroy_inode(struct inode *inode)
> +static void sock_cache_free_rcu(struct rcu_head *rcu)
>  {
> -       struct socket_alloc *ei;
> -       struct socket_wq *wq;
> -
> -       ei = container_of(inode, struct socket_alloc, vfs_inode);
> -       wq = rcu_dereference_protected(ei->socket.wq, 1);
> -       kfree_rcu(wq, rcu);
> +       struct socket_alloc *ei =
> +               container_of(rcu, struct socket_alloc, vfs_inode.i_rcu);
>  	kmem_cache_free(sock_inode_cachep, ei);
>  }
>  
> +static void sock_destroy_inode(struct inode *inode)
> +{
> +       call_rcu(&inode->i_rcu, sock_cache_free_rcu);
> +}
> +
>  static void init_once(void *foo)
>  {
>  	struct socket_alloc *ei = (struct socket_alloc *)foo;
> @@ -573,7 +567,7 @@ void sock_release(struct socket *sock)
>  		module_put(owner);
>  	}
>  
> -       if (rcu_dereference_protected(sock->wq, 1)->fasync_list)
> +       if (sock->wq.fasync_list)
>  		pr_err("%s: fasync list not empty!\n", __func__);
>  
>  	this_cpu_sub(sockets_in_use, 1);
> @@ -1044,7 +1038,7 @@ static int sock_fasync(int fd, struct file *filp,
> int on)
>  		return -EINVAL;
>  
>  	lock_sock(sk);
> -       wq = rcu_dereference_protected(sock->wq, sock_owned_by_user(sk));
> +       wq = &sock->wq;
>  	fasync_helper(fd, filp, on, &wq->fasync_list);
>  
>  	if (!wq->fasync_list)
> @@ -1065,7 +1059,7 @@ int sock_wake_async(struct socket *sock, int how,
> int band)
>  	if (!sock)
>  		return -1;
>  	rcu_read_lock();
> -       wq = rcu_dereference(sock->wq);
> +       wq = &sock->wq;
>  	if (!wq || !wq->fasync_list) {
>  		rcu_read_unlock();
>  		return -1;
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278310

On Thu, 2015-11-26 at 14:32 +0100, Hannes Frederic Sowa wrote:
> Hannes Frederic Sowa <hannes@stressinduktion.org> writes:
> 
> 
> > I have seen filesystems already doing so in .destroy_inode, that's why I
> > am asking. The allocation happens the same way as we do with sock_alloc,
> > e.g. shmem. I actually thought that struct inode already provides an
> > rcu_head for exactly that reason.
> 
> E.g.:

> +static void sock_destroy_inode(struct inode *inode)
> +{
> +	call_rcu(&inode->i_rcu, sock_cache_free_rcu);
> +}

I guess you missed few years back why we had to implement
SLAB_DESTROY_BY_RCU for TCP sockets to not destroy performance.

By adding RCU grace period before reuse of this inode (about 640 bytes
today), you are asking the CPU to evict from its cache precious content,
and slow down some workloads, adding lot of ram pressure, as the cpu
allocating a TCP socket will have to populate its cache for a cold
inode.

The reason we put in a small object the RCU protected fields should be
pretty clear.

Do not copy code that people wrote in other layers without understanding
the performance implications.

Thanks.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278348

On Thu, 2015-11-26 at 18:03 +0100, Hannes Frederic Sowa wrote:

> My rationale was like this: we already have rcu to free the wq, so we
> don't add any more callbacks as current code. sock_alloc is right now
> 1136 bytes, which is huge, like 18 cachelines. I wouldn't think it does
> matter a lot as we thrash anyway. tcp_sock is like 45 cachelines right
> now, hui.

Again, these speculations are really killing me.

Can you wait I send my patches ?

They work just great, and there is no risk of perf regression with added
RCU freeing of inodes :(

It is Thanks Giving in the US, vacation time.

Thanks.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278355

On Thu, Nov 26, 2015, at 18:09, Eric Dumazet wrote:
> On Thu, 2015-11-26 at 18:03 +0100, Hannes Frederic Sowa wrote:
> 
> > My rationale was like this: we already have rcu to free the wq, so we
> > don't add any more callbacks as current code. sock_alloc is right now
> > 1136 bytes, which is huge, like 18 cachelines. I wouldn't think it does
> > matter a lot as we thrash anyway. tcp_sock is like 45 cachelines right
> > now, hui.
> 
> Again, these speculations are really killing me.
>
> Can you wait I send my patches ?

Eric, I wasn't targeting upstream. I just had this idea and wanted to
show you that it is possible. I didn't send an official patch and
wouldn't do that without proper performance analysis.

> They work just great, and there is no risk of perf regression with added
> RCU freeing of inodes :(

I agree. Your patch is certainly the best thing for net while this
complete rcuification should never ever hit net.

> It is Thanks Giving in the US, vacation time.

Enjoy the time! :)

Bye,
Hannes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278350

On Thu, Nov 26, 2015, at 16:51, Eric Dumazet wrote:
> On Thu, 2015-11-26 at 14:32 +0100, Hannes Frederic Sowa wrote:
> > Hannes Frederic Sowa <hannes@stressinduktion.org> writes:
> > 
> > 
> > > I have seen filesystems already doing so in .destroy_inode, that's why I
> > > am asking. The allocation happens the same way as we do with sock_alloc,
> > > e.g. shmem. I actually thought that struct inode already provides an
> > > rcu_head for exactly that reason.
> > 
> > E.g.:
> 
> > +static void sock_destroy_inode(struct inode *inode)
> > +{
> > +	call_rcu(&inode->i_rcu, sock_cache_free_rcu);
> > +}
> 
> I guess you missed few years back why we had to implement
> SLAB_DESTROY_BY_RCU for TCP sockets to not destroy performance.

I think I wasn't even subscribed to netdev@ at that time, so I probably
missed it. Few years back is 7. :}

> By adding RCU grace period before reuse of this inode (about 640 bytes
> today), you are asking the CPU to evict from its cache precious content,
> and slow down some workloads, adding lot of ram pressure, as the cpu
> allocating a TCP socket will have to populate its cache for a cold
> inode.

My rationale was like this: we already have rcu to free the wq, so we
don't add any more callbacks as current code. sock_alloc is right now
1136 bytes, which is huge, like 18 cachelines. I wouldn't think it does
matter a lot as we thrash anyway. tcp_sock is like 45 cachelines right
now, hui.

Also isn't the reason why slub exists so it can track memory regions
per-cpu.

Anyway, I am only speculating why it could be tried. I probably need to
do some performance experiments.

> The reason we put in a small object the RCU protected fields should be
> pretty clear.

Yes, I thought about that.

> Do not copy code that people wrote in other layers without understanding
> the performance implications.

Duuh. :)

Bye,
Hannes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278365

On Thu, 2015-11-26 at 18:03 +0100, Hannes Frederic Sowa wrote:

> Also isn't the reason why slub exists so it can track memory regions
> per-cpu.

call_rcu() and kfree_rcu() will add a grace period (multiple ms) where
the cpu will likely evict from its caches the data contained in the
'about to be freed' objects, defeating the SLUB/SLAB ability to quickly
reuse a freed and hot object (LIFO)

This is one of the major RCU drawback : Force a FIFO behavior in object
reuse while LIFO one is much better for data locality, especially with
per-cpu lists.

Another problem is a slightly bigger working set size, which can hurt
some workloads that used to exactly fit cpu caches.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1276952

Eric Dumazet <edumazet@google.com> writes:
> On Tue, Nov 24, 2015 at 3:34 PM, Rainer Weikusat
> <rweikusat@mobileactivedefense.com> wrote:
>> Eric Dumazet <edumazet@google.com> writes:
>>> On Tue, Nov 24, 2015 at 6:18 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>>> Hello,
>>>>
>>>> The following program triggers use-after-free in sock_wake_async:
>>
>> [...]
>>
>>>> void *thr1(void *arg)
>>>> {
>>>>         syscall(SYS_close, r2, 0, 0, 0, 0, 0);
>>>>         return 0;
>>>> }
>>>>
>>>> void *thr2(void *arg)
>>>> {
>>>>         syscall(SYS_write, r3, 0x20003000ul, 0xe7ul, 0, 0, 0);
>>>>         return 0;
>>>> }
>>
>> [...]
>>
>>>>         pthread_t th[3];
>>>>         pthread_create(&th[0], 0, thr0, 0);
>>>>         pthread_create(&th[1], 0, thr1, 0);
>>>>         pthread_create(&th[2], 0, thr2, 0);
>>>>         pthread_join(th[0], 0);
>>>>         pthread_join(th[1], 0);
>>>>         pthread_join(th[2], 0);
>>>>         return 0;
>>>> }
>>
>> [...]
>>
>>> Looks like commit 830a1e5c212fb3fdc83b66359c780c3b3a294897 should be reverted ?
>>>
>>> commit 830a1e5c212fb3fdc83b66359c780c3b3a294897
>>> Author: Benjamin LaHaise <benjamin.c.lahaise@intel.com>
>>> Date:   Tue Dec 13 23:22:32 2005 -0800
>>>
>>>     [AF_UNIX]: Remove superfluous reference counting in unix_stream_sendmsg
>>>
>>>     AF_UNIX stream socket performance on P4 CPUs tends to suffer due to a
>>>     lot of pipeline flushes from atomic operations.  The patch below
>>>     removes the sock_hold() and sock_put() in unix_stream_sendmsg().  This
>>>     should be safe as the socket still holds a reference to its peer which
>>>     is only released after the file descriptor's final user invokes
>>>     unix_release_sock().  The only consideration is that we must add a
>>>     memory barrier before setting the peer initially.
>>>
>>>     Signed-off-by: Benjamin LaHaise <benjamin.c.lahaise@intel.com>
>>>     Signed-off-by: David S. Miller <davem@davemloft.net>
>>
>> JFTR: This seems to be unrelated. (As far as I understand this), the
>> problem is that sk_wake_async accesses sk->sk_socket. That's invoked via
>> the
>>
>> other->sk_data_ready(other)
>>
>> in unix_stream_sendmsg after an
>>
>> unix_state_unlock(other);
>>
>> because of this, it can race with the code in unix_release_sock clearing
>> this pointer (via sock_orphan). The structure this pointer points to is
>> freed via iput in sock_release (net/socket.c) after the af_unix release
>> routine returned (it's really one part of a "twin structure" with the
>> socket inode being the other).
>>
>> A quick way to test if this was true would be to swap the
>>
>> unix_state_unlock(other);
>> other->sk_data_ready(other);
>>
>> in unix_stream_sendmsg and in case it is, a very 'hacky' fix could be to
>> put a pointer to the socket inode into the struct unix_sock, do an iget
>> on that in unix_create1 and a corresponding iput in
>> unix_sock_destructor.
>
> This is interesting, but is not the problem or/and the fix.
>
> We are supposed to own a reference on the 'other' socket or make sure
> it cannot disappear under us.

The af_unix part of this, yes, ie, what gets allocated in
unix_create1. But neither the socket inode nor the struct sock
originally passed to unix_create. Since these are part of the same
umbrella structure, they'll both be freed as consequence of the
sock_release iput. As far as I can tell (I don't claim that I'm
necessarily right on this, this is just the result of spending ca 2h
reading the code with the problem report in mind and looking for
something which could cause it), doing a sock_hold on the unix peer of
the socket in unix_stream_sendmsg is indeed not needed, however, there's
no additional reference to the inode or the struct sock accompanying it,
ie, both of these will be freed by unix_release_sock. This also affects
unix_dgram_sendmsg.

It's also easy to verify: Swap the unix_state_lock and
other->sk_data_ready and see if the issue still occurs. Right now (this
may change after I had some sleep as it's pretty late for me), I don't
think there's another local fix: The ->sk_data_ready accesses a
pointer after the lock taken by the code which will clear and
then later free it was released.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web