Message-ID: <cd8907dc-0319-6c04-271c-489ca4550579@intel.com>
Date:   Mon, 13 Feb 2023 16:05:59 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v2] l2tp: Avoid possible recursive deadlock in
 l2tp_tunnel_register()
Content-Language: en-US
To:     Shigeru Yoshida <syoshida@redhat.com>
CC:     <jchapman@katalix.com>, <davem@davemloft.net>,
        <edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>,
        <gnault@redhat.com>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20230212162623.2301597-1-syoshida@redhat.com>
From:   Alexander Lobakin <alexandr.lobakin@intel.com>
In-Reply-To: <20230212162623.2301597-1-syoshida@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VGdQV2xrUU16d0s2SmVCUXd1YVY3MEhkb0ZNNFQ4cDBJSjZqSGxMeDNZVUs0?=
 =?utf-8?B?dFI0aW1jR2VXSjJwVmFvV2VNaEwwb0ZVR3V6ZDFIakpvSUFxaWFpZEJWcTdF?=
 =?utf-8?B?SGtYUDhHNFloM04wVjFtbmJOQ0FEQlpuOWFVMUxZWnBCaWR6T1JldERtQzR3?=
 =?utf-8?B?bThQbFRXRlMxUzlwTlF1cU5MSFV6QnR1TWtITG9oTWd3cmJpSlRwS3RFOUZP?=
 =?utf-8?B?T2ZMLzBYTGxseXZVV2VIME9rTmovaDRSbHRhMEk5aGVhVUFlYzRGaE5FcC8z?=
 =?utf-8?B?UlVDeTJJV2NUUVhpanFKbFAzRzdhdDZQUWNDU3AzNUZvRytHc0gyZTYrWVR6?=
 =?utf-8?B?QU9LbHQ0VjZTZTdMS0V6d2NoNnRPbmQwWUd1L3ZVRjQ2eEd4c0VvYWZlRDlw?=
 =?utf-8?B?WjhMUDk1Nzliczl6UEptTEc1TlJON2lJRDdTZExMQ29INEhHRlE2MnNMYS9Q?=
 =?utf-8?B?RGprNGhQbTQ2UzZ2TzZXaGkwRHEzaTlVWFgzbDk3N2dMUXV0blhuWUhTOGtM?=
 =?utf-8?B?SXJrczIrYUhGdEhRU0tneWdSTS9RTVo0cmFlUnpIekFzbHJxbHNlZlNRUURn?=
 =?utf-8?B?WWF2bjNMcExXeEY1MjY3S0JIajNaOFpOQmxOdzBxeHZqR2RuWmx2ZklMbWdk?=
 =?utf-8?B?T2ZKSHE3aTNVRnJjbjRQYVp4ZTFYU1hlSytwUG9sbUtqVG9YcmlNMkdHYkR6?=
 =?utf-8?B?MG13L0dzYUMwbnhUZTJoR2VEVnlydS9JTVE4eTF2V2ZGd0dFUkltYnlQcjlZ?=
 =?utf-8?B?d2hmemt2MmNGRS9lWUI5Vy82SXkwdU5yeXRtMjNlSWRreFUrN0dWSisyWnU1?=
 =?utf-8?B?OXZxVWRLVHpRWWpWSUpSSlZ5YmFPNFZCcTJidm52dE9zdnNtQkZJRlNCbXZW?=
 =?utf-8?B?Z0Y4bnpIU3N6VWdvTWtTZmE1UU0xTUg4MWxGWWdaSXYrdmd2cnFWNzJ6bzFK?=
 =?utf-8?B?QjZKZWRmQkQ2bzB3L1VZVnB2K2sydEc4TXJmYnVQMUNZclVqdGlwejAxVVhw?=
 =?utf-8?B?eWRCQktsU2luaFdrMUY2UHRJUmVwMDhtY2s0ajZRTEhJdHVWT1FkNWIxNUor?=
 =?utf-8?B?dmNyTTRwY2xvWWw5TjZzQVZEUm1HVmw2Z01iMndOdWhucWRzMTVoaW1zR0E0?=
 =?utf-8?B?MHhNRkdGeHRaUzhJN1k2K25JWnJPZGFkeG1vbDFxM3F5OEUwVXBhYWQ4RVdS?=
 =?utf-8?B?N0h6clJYTTl3RVJVQnlBRHhMSVpaS1lqb1dUa3NLT05pdGcvQ2ZjZUNYMUxW?=
 =?utf-8?B?QVZyZlpla1BydE95a1BaalNqY29TSnN5M3JEQ3JORlMrekxDNnN5U1Z4R2RM?=
 =?utf-8?B?UzRYR09LeERJOEZlQ1N3T1NwQmlUWWVPcmhlejAyYVpEN2JqclQwWmR2UUxv?=
 =?utf-8?B?RkQ3Wld2UUpXLzhTeUd5NmkrQXBNSThENWlSWUhwVlpOMUtEY0RWak9qQUta?=
 =?utf-8?B?NEsveHlEZ091ck1xTHYzVTRGc0xlVENQdytEYjdDMkVFMWZxSFoyY1FFN1VB?=
 =?utf-8?B?cHRPb2VVOHdWMUgrd1pJYUs2djFmd25sVCtTc3lRcGRKTnNKbWh2UktxNnkz?=
 =?utf-8?B?dUNMVWFsL1hUU0Y4cVE3ajZqMzE3ZUh1RDU3UjJDOURIbHhKREtFN1V6WnVT?=
 =?utf-8?B?eEtJdXY1cThqdDN0V29zbTNHdWFPTGNteVNJQSt4bUVFenVWUHgxblQ0elkw?=
 =?utf-8?B?dmNKNko1aFhVaG54ODAxRGEzTkIvdURDN21VUncvN2dXMWNYY25wZ1I0Lyt1?=
 =?utf-8?B?cGNDVnkwL2ZnMFRjQ2YraVRyR3lHTXNxcFROT1BkS2UzdXVJa2Q4S3VsZUti?=
 =?utf-8?B?VG1vZG9oYm9OVko3aGJVRGtGbDVZbFJjcGxsSVJHUFlZeXVOcG5SQjJQV2Fo?=
 =?utf-8?B?QUNnYkgwSk9Xa2ZsNTNLK3ZWMHVSaHZsYS9MZ2tnZ3ZreEh0a0crT2JTVjI3?=
 =?utf-8?B?b3RsZXcyQ3hHMUc2RVZXejhIcnFlTGRzZTU1V01MMHN3bkZ0QnE2c3hvUVdX?=
 =?utf-8?B?UEhvRUhTK2FFUmw1bGJSMlNIc2Fnb2t2SlRhVTBFZVZlL2tRdVgrSVVjRjBN?=
 =?utf-8?B?aWJBS1ZFU1U3cmZmTlVzS2I1WngxWFZUdkVNN0ZFSWJvalF0RThINnlNSHQy?=
 =?utf-8?B?NGZkb25XaWp1WjVMNjhWcWNMdzEycVcwNER5S0dIakVyUkcrQUp5UCt1Ujls?=
 =?utf-8?B?SkE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3497b136-ae59-4621-f475-08db0dd3f5f8
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Feb 2023 15:07:02.2175
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: IWB+IEahCC0IXB/kcrwQeuGiMmjqJQmRjGF9lZJtrpKK5arZRVQynfrltRvzWwGhJXpkIhUYz17J3aSiszfAfqIH/qW39TdD32XoBQwCkwg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB7348
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Shigeru Yoshida <syoshida@redhat.com>
Date: Mon, 13 Feb 2023 01:26:23 +0900

> When a file descriptor of pppol2tp socket is passed as file descriptor
> of UDP socket, a recursive deadlock occurs in l2tp_tunnel_register().
> This situation is reproduced by the following program:

[...]

> +static struct l2tp_tunnel *pppol2tp_tunnel_get(struct net *net,
> +					       struct l2tp_connect_info *info,
> +					       bool *new_tunnel)
> +{
> +	struct l2tp_tunnel *tunnel;
> +	int error;
> +
> +	*new_tunnel = false;
> +
> +	tunnel = l2tp_tunnel_get(net, info->tunnel_id);
> +
> +	/* Special case: create tunnel context if session_id and
> +	 * peer_session_id is 0. Otherwise look up tunnel using supplied
> +	 * tunnel id.
> +	 */
> +	if (!info->session_id && !info->peer_session_id) {
> +		if (!tunnel) {

This `if` is the sole thing the outer `if` contains, could we combine them?

> +			struct l2tp_tunnel_cfg tcfg = {
> +				.encap = L2TP_ENCAPTYPE_UDP,
> +			};
> +
> +			/* Prevent l2tp_tunnel_register() from trying to set up
> +			 * a kernel socket.
> +			 */
> +			if (info->fd < 0)
> +				return ERR_PTR(-EBADF);
> +
> +			error = l2tp_tunnel_create(info->fd,
> +						   info->version,

This fits into the prev line.

> +						   info->tunnel_id,
> +						   info->peer_tunnel_id, &tcfg,
> +						   &tunnel);
> +			if (error < 0)
> +				return ERR_PTR(error);
> +
> +			l2tp_tunnel_inc_refcount(tunnel);
> +			error = l2tp_tunnel_register(tunnel, net, &tcfg);
> +			if (error < 0) {
> +				kfree(tunnel);
> +				return ERR_PTR(error);
> +			}
> +
> +			*new_tunnel = true;
> +		}
> +	} else {
> +		/* Error if we can't find the tunnel */
> +		if (!tunnel)
> +			return ERR_PTR(-ENOENT);

[...]

Thanks,
Olek