Message-ID: <12305071-c136-f39f-9450-bdaad08137b2@gmail.com>
Date:   Tue, 28 Feb 2023 09:48:11 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH] net: tls: fix possible race condition between
 do_tls_getsockopt_conf() and do_tls_setsockopt_conf()
To:     Jakub Kicinski <kuba@kernel.org>
Cc:     Sabrina Dubroca <sd@queasysnail.net>,
        Florian Westphal <fw@strlen.de>, borisp@nvidia.com,
        john.fastabend@gmail.com, davem@davemloft.net, edumazet@google.com,
        pabeni@redhat.com, davejwatson@fb.com, aviadye@mellanox.com,
        ilyal@mellanox.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230224105811.27467-1-hbh25y@gmail.com>
 <20230224120606.GI26596@breakpoint.cc> <20230224105508.4892901f@kernel.org>
 <Y/kck0/+NB+Akpoy@hog> <20230224130625.6b5261b4@kernel.org>
 <Y/kwyS2n4uLn8eD0@hog> <20230224141740.63d5e503@kernel.org>
 <52faaa10-f3e4-bca9-4bff-6f1ea7d26593@gmail.com>
 <20230227110750.6988fca5@kernel.org>
Content-Language: en-US
From:   Hangyu Hua <hbh25y@gmail.com>
In-Reply-To: <20230227110750.6988fca5@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 28/2/2023 03:07, Jakub Kicinski wrote:
> On Mon, 27 Feb 2023 11:26:18 +0800 Hangyu Hua wrote:
>> In order to reduce ambiguity, I think it may be a good idea only to
>> lock do_tls_getsockopt_conf() like we did in do_tls_setsockopt()
>>
>> It will look like:
>>
>> static int do_tls_getsockopt(struct sock *sk, int optname,
>> 			     char __user *optval, int __user *optlen)
>> {
>> 	int rc = 0;
>>
>> 	switch (optname) {
>> 	case TLS_TX:
>> 	case TLS_RX:
>> +		lock_sock(sk);
>> 		rc = do_tls_getsockopt_conf(sk, optval, optlen,
>> 					    optname == TLS_TX);
>> +		release_sock(sk);
>> 		break;
>> 	case TLS_TX_ZEROCOPY_RO:
>> 		rc = do_tls_getsockopt_tx_zc(sk, optval, optlen);
>> 		break;
>> 	case TLS_RX_EXPECT_NO_PAD:
>> 		rc = do_tls_getsockopt_no_pad(sk, optval, optlen);
>> 		break;
>> 	default:
>> 		rc = -ENOPROTOOPT;
>> 		break;
>> 	}
>> 	return rc;
>> }
>>
>> Of cause, I will clean the lock in do_tls_getsockopt_conf(). What do you
>> guys think?
> 
> I'd suggest to take the lock around the entire switch statement.

I get it. I will send a v2 later.

Thanks,
Hangyu