On 03/08/2019 02:40 PM, Guillaume Nault wrote:
> On Fri, Mar 08, 2019 at 02:34:07PM -0800, Eric Dumazet wrote:
>>
>>
>> On 03/08/2019 02:22 PM, Guillaume Nault wrote:
>>> On Fri, Mar 08, 2019 at 01:33:02PM -0800, Eric Dumazet wrote:
>>>>
>>>>
>>>> On 03/08/2019 01:09 PM, Guillaume Nault wrote:
>>>>> @@ -216,7 +216,12 @@ struct sock *tcp_get_cookie_sock(struct sock *sk,
>>>>> struct sk_buff *skb,
>>>>> refcount_set(&req->rsk_refcnt, 1);
>>>>> tcp_sk(child)->tsoffset = tsoff;
>>>>> sock_rps_save_rxhash(child, skb);
>>>>> - inet_csk_reqsk_queue_add(sk, req, child);
>>>>> + if (!inet_csk_reqsk_queue_add(sk, req, child)) {
>>>>> + bh_unlock_sock(child);
>>>>> + sock_put(child);
>>>>> + child = NULL;
>>>>> + reqsk_put(req);
>>>>
>>>> Since we use reqsk_free(req) in the same function, we can use
>>>> reqsk_free(req)
>>>> here as well ?
>>>>
>>> That was my first approach, but reqsk_free() doesn't like it:
>>>
>>> static inline void reqsk_free(struct request_sock *req)
>>> {
>>> /* temporary debugging */
>>> WARN_ON_ONCE(refcount_read(&req->rsk_refcnt) != 0);
>>> ...
>>> }
>>
>> Oh right, there is this refcount_set(&req->rsk_refcnt, 1) before the call
>> to inet_csk_reqsk_queue_add(sk, req, child);
>>
>> So just change the TFO case only :)
>>
> Well.. refcount is 1 in the TFO case too.
Arg...
>
> Long term, do we want to keep the WARN_ON_ONCE()? If so, we should
> probably remove the comment.
We want to keep the warning.
We do not have a way to tell if the req was ever inserted in a hash table, so
better play safe.
Signed-off-by: Eric Dumazet <eduma...@google.com>
Thanks !
