netdev - Re: [PATCH v2 net-next 2/3] net/smc: Limits backlog connections

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 2 Feb 2022 22:01:46 +0800
From:   "D. Wythe" <alibuda@...ux.alibaba.com>
To:     Tony Lu <tonylu@...ux.alibaba.com>
Cc:     kgraul@...ux.ibm.com, kuba@...nel.org, davem@...emloft.net,
        netdev@...r.kernel.org, linux-s390@...r.kernel.org,
        linux-rdma@...r.kernel.org, matthieu.baerts@...sares.net
Subject: Re: [PATCH v2 net-next 2/3] net/smc: Limits backlog connections

The overhead will certainly exist, but compared with the benefits, I 
think it should be acceptable. If you do care, maybe we can add a switch 
to control it.


> I am wondering if there would introduce more overhead, compared with
> original implement?
> 
>> +
>> +drop:
>> +	dst_release(dst);
>> +	tcp_listendrop(sk);
>> +	return NULL;
>> +}
>> +
>>   static struct smc_hashinfo smc_v4_hashinfo = {
>>   	.lock = __RW_LOCK_UNLOCKED(smc_v4_hashinfo.lock),
>>   };
>> @@ -1491,6 +1519,9 @@ static void smc_listen_out(struct smc_sock *new_smc)
>>   	struct smc_sock *lsmc = new_smc->listen_smc;
>>   	struct sock *newsmcsk = &new_smc->sk;
>>   
>> +	if (tcp_sk(new_smc->clcsock->sk)->syn_smc)
>> +		atomic_dec(&lsmc->smc_pendings);
>> +
>>   	if (lsmc->sk.sk_state == SMC_LISTEN) {
>>   		lock_sock_nested(&lsmc->sk, SINGLE_DEPTH_NESTING);
>>   		smc_accept_enqueue(&lsmc->sk, newsmcsk);
>> @@ -2096,6 +2127,9 @@ static void smc_tcp_listen_work(struct work_struct *work)
>>   		if (!new_smc)
>>   			continue;
>>   
>> +		if (tcp_sk(new_smc->clcsock->sk)->syn_smc)
>> +			atomic_inc(&lsmc->smc_pendings);
>> +
>>   		new_smc->listen_smc = lsmc;
>>   		new_smc->use_fallback = lsmc->use_fallback;
>>   		new_smc->fallback_rsn = lsmc->fallback_rsn;
>> @@ -2163,6 +2197,15 @@ static int smc_listen(struct socket *sock, int backlog)
>>   	smc->clcsock->sk->sk_data_ready = smc_clcsock_data_ready;
>>   	smc->clcsock->sk->sk_user_data =
>>   		(void *)((uintptr_t)smc | SK_USER_DATA_NOCOPY);
>> +
>> +	/* save origin ops */
>> +	smc->ori_af_ops = inet_csk(smc->clcsock->sk)->icsk_af_ops;
>> +
>> +	smc->af_ops = *smc->ori_af_ops;
>> +	smc->af_ops.syn_recv_sock = smc_tcp_syn_recv_sock;
>> +
>> +	inet_csk(smc->clcsock->sk)->icsk_af_ops = &smc->af_ops;
> 


Only save syn_recv_sock? Maybe this comment is confusing，
  ‘Copy the origin ops’ is better, the origin ops is pointer to a const 
structure, we must copy it all, and repointer it to our structure. so 
the copy/save is necessary.

Thanks.

> Consider to save syn_recv_sock this field only? There seems no need to
> save this ops all.
> 
> Thank you,
> Tony Lu