Re: [PATCH net] net: sctp: fix copying sk_v6_rcv_saddr in sctp_v6_create_accept_sk

[Daniel Borkmann <dborkman@redhat.com>]

On 11/21/2013 07:49 AM, wangweidong wrote:
> On 2013/11/19 23:38, Vlad Yasevich wrote:
>> On 11/19/2013 05:51 AM, Daniel Borkmann wrote:
>>> Wang reported an issue that lksctp's test_getname_v6 seems to fail.
>>>
>>> The issue is that we do not copy sk_v6_rcv_saddr over to the new
>>> socket, although the comment above says so regarding rcv_saddr.
>>>
>>> Commit 914e1c8b6980 ("sctp: Inherit all socket options from parent
>>> correctly.") originally moved that over to sctp_copy_sock(), but
>>> after commit efe4208f47f9 ("ipv6: make lookups simpler and faster")
>>> this no longer holds and the actual value of sk_v6_rcv_saddr was
>>> no longer being migrated.
>>>
>>> With this patch, the lksctp test suite passes again for IPv6.
>>>
>>> Fixes: efe4208f47f9 ("ipv6: make lookups simpler and faster")
>>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>>> Cc: Eric Dumazet <eric.dumazet@gmail.com>
>>> ---
>>>   net/sctp/ipv6.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>>> index 7567e6f..be08592 100644
>>> --- a/net/sctp/ipv6.c
>>> +++ b/net/sctp/ipv6.c
>>> @@ -661,6 +661,7 @@ static struct sock *sctp_v6_create_accept_sk(struct sock *sk,
>>>   	 * and getpeername().
>>>   	 */
>>>   	sctp_v6_to_sk_daddr(&asoc->peer.primary_addr, newsk);
>>> +	newsk->sk_v6_rcv_saddr = sk->sk_v6_rcv_saddr;
>>>
>>>   	sk_refcnt_debug_inc(newsk);
>>>
>>>
>>
>> This fixes the issue for the accept() case, but the bug is still there
>> in the peeloff case.
>>
>> I think you should make sctp_copy_sock() use sock_copy() for now.  It
>> looks like it will catch all the cases.
>>
>
> I copied the part of the sock_copy() codes into sctp_v6_create_accept_sk instead of
> sctp_copy_sock for testing. Like this:
>
> --------------
>
> @@ -645,7 +645,11 @@ static struct sock *sctp_v6_create_accept_sk(struct sock *sk,
>
>          sock_init_data(NULL, newsk);
>
> -       sctp_copy_sock(newsk, sk, asoc);
> +       memcpy(newsk, sk, offsetof(struct sock, sk_dontcopy_begin));
> +
> +       memcpy(&newsk->sk_dontcopy_end, &sk->sk_dontcopy_end,
> +              sk->sk_prot->obj_size - offsetof(struct sock, sk_dontcopy_end));
> +
>          sock_reset_flag(sk, SOCK_ZAPPED);
>
>          newsctp6sk = (struct sctp6_sock *)newsk;
>
> --------------
>
> when I did the test_getname_v6 of lksctp tools. It would hang. And I used the ps aux,
> I can see the process got a D+ STAT.
>
> BTW, I test it in the virtual machine.
>
> So I think: when we use the sock_copy or which calls it, maybe we will get the problem here.
> Is there I do something wrong?

Sorry, got distracted with other things in the mean time.

I will have a closer look at it very soon, so that we can find a proper fix.

> Thanks.
>
>> We can then look at possibly getting rid of sctp_copy_sock() for net-next.
>>
>> -vlad
>>
>>
>>
>
>
> --
> 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
>