From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED, USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EAAFDC282CE for ; Wed, 24 Apr 2019 18:21:16 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id BC38E20685 for ; Wed, 24 Apr 2019 18:21:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1733230AbfDXSVP (ORCPT ); Wed, 24 Apr 2019 14:21:15 -0400 Received: from mail-wr1-f68.google.com ([209.85.221.68]:39604 "EHLO mail-wr1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726473AbfDXSVP (ORCPT ); Wed, 24 Apr 2019 14:21:15 -0400 Received: by mail-wr1-f68.google.com with SMTP id a9so8900986wrp.6 for ; Wed, 24 Apr 2019 11:21:13 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=iEd0pWV7yJDALaE8HvhJ/7kSxHuuFkkj19XYdtQV3OA=; b=fXTbwIAXrmZx74T0U3ZV91jebWWKdgr5BUbHQLptHp206oCRMS5wteP0eg/xS9oT2X HWgNo2h4zB1FzzIXelWj/MHU5hG1YUoC5jIJSWjO/PN7BNuAqFgo4FVjmu6mC8kxDz/n JPQAC2pO2ECiJ+1sagQ6YLSxq0XgiaBG7Z+kvwdxhHMprYCD2G0RpGb9imF/IUMaP7Dv 5q9YV5931cKzrymy6KqKRVctRfbVqbccwRIChvns8xoac3HUEUVP3GopyRYxtsQvPqM4 GURTtF1jWkZM9UOANx7oVNO4nt6quuQksYwqYppEsZTaQlnSvoiBEGnxZPlNeNIjXN5A 6d5A== X-Gm-Message-State: APjAAAWOqqcW1EBKJjT9uv448Wzk6DbqhjGadpSAy/JjDzecsWDWPjom obpFPx7mIqYBW3lhFNNT9uhdKQ== X-Google-Smtp-Source: APXvYqyLHpNBs1G2rdWRkOVTyjwnhp5obUGWo34HFCcPNcvfO5fO50zIbjwdXnHlxLHLiVano2bNXw== X-Received: by 2002:adf:ea88:: with SMTP id s8mr1246963wrm.300.1556130072746; Wed, 24 Apr 2019 11:21:12 -0700 (PDT) Received: from pc-2.home (2a01cb05850ddf00045dd60e6368f84b.ipv6.abo.wanadoo.fr. [2a01:cb05:850d:df00:45d:d60e:6368:f84b]) by smtp.gmail.com with ESMTPSA id o5sm17934128wmc.16.2019.04.24.11.21.11 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 24 Apr 2019 11:21:11 -0700 (PDT) Date: Wed, 24 Apr 2019 20:21:10 +0200 From: Guillaume Nault To: Eric Dumazet Cc: "David S . Miller" , netdev , Eric Dumazet , James Chapman Subject: Re: [PATCH net] l2tp: use rcu_dereference_sk_user_data() in l2tp_udp_encap_recv() Message-ID: <20190424182108.GB17274@pc-2.home> References: <20190423164326.108651-1-edumazet@google.com> <20190424095809.GA17274@pc-2.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.11.3 (2019-02-01) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Wed, Apr 24, 2019 at 04:33:47AM -0700, Eric Dumazet wrote: > On Wed, Apr 24, 2019 at 2:58 AM Guillaume Nault wrote: > > > > On Tue, Apr 23, 2019 at 09:43:26AM -0700, Eric Dumazet wrote: > > > Canonical way to fetch sk_user_data from an encap_rcv() handler called > > > from UDP stack in rcu protected section is to use rcu_dereference_sk_user_data(), > > > otherwise compiler might read it multiple times. > > > > > That reminds me the more general problem we have with ->sk_user_data: > > https://lore.kernel.org/netdev/20180117.142538.1972806008716856078.davem@davemloft.net/ > > > > We're not even guarateed that ->sk_user_data points to a struct l2tp_tunnel > > (some external modules can still probably override it). > > > > RCU rules should prevail. > > An RCU grace period must be enforced if the same UDP socket has its > sk_user_data changed. > > Normally, UDP socket observes an RCU grace period at close/destroy > time. (SOCK_RCU_FREE) > > If we detect the same UDP socket has not been closed between clearing > sk_user_data > and setting it again to a non NULL value, we must insert a synchronize_rcu() > > A generic change could look like the following (but we must check that all > rcu_assign_sk_user_data() callers can sleep) > It seems that callers of rcu_assign_sk_user_data() can indeed sleep. But I think we have more problems. Not all code paths treat ->sk_user_data as RCU pointers (IIUC that's why we created the __sk_user_data() macro, instead of just redefining ->sk_user_data as "void __rcu *"). But even if RCU rules were respected for all accesses, we'd need to ensure consistent protection for the update side. And then, we'd need to make sure that ->sk_user_data is in sync with the encap_rcv() callback (or whatever actually uses the data pointed to). Otherwise a module could treat ->sk_user_data as a struct foo pointer while it actually points to a struct bar. For example, a quick look at net/sunrpc/svcsock.c seems to indicate that svc_addsock() would accept any (unconnected) UDP socket and pass it to svc_addsock(), which in turn would override ->sk_user_data with a struct svc_sock pointer. If the socket was previously set up by L2TP, then we'd end up with ->sk_user_data pointing to a svc_sock structure, but ->encap_rcv still pointing to l2tp_udp_encap_recv(). That's going to give unexpected results when l2tp_udp_encap_recv() will dereference ->sk_user_data to access (what it believes to be) its tunnel structure. > diff --git a/include/net/sock.h b/include/net/sock.h > index 784cd19d5ff76b53865f79d4580f3fa269fa2408..3cf0dfd9d83a956220d69138339561b62407addd > 100644 > --- a/include/net/sock.h > +++ b/include/net/sock.h > @@ -522,7 +522,6 @@ enum sk_pacing { > #define __sk_user_data(sk) ((*((void __rcu **)&(sk)->sk_user_data))) > > #define rcu_dereference_sk_user_data(sk) > rcu_dereference(__sk_user_data((sk))) > -#define rcu_assign_sk_user_data(sk, ptr) > rcu_assign_pointer(__sk_user_data((sk)), ptr) > > /* > * SK_CAN_REUSE and SK_NO_REUSE on a socket mean that the socket is OK > @@ -811,6 +810,7 @@ enum sock_flags { > SOCK_FILTER_LOCKED, /* Filter cannot be changed anymore */ > SOCK_SELECT_ERR_QUEUE, /* Wake select on error queue */ > SOCK_RCU_FREE, /* wait rcu grace period in sk_destruct() */ > + SOCK_USER_DATA_SET, > SOCK_TXTIME, > SOCK_XDP, /* XDP is attached */ > SOCK_TSTAMP_NEW, /* Indicates 64 bit timestamps always */ > @@ -2582,4 +2582,15 @@ static inline bool sk_dev_equal_l3scope(struct > sock *sk, int dif) > return false; > } > > +static inline void rcu_assign_sk_user_data(struct sock *sk, void *ptr) > +{ > + if (ptr) { > + might_sleep(); > + if (sock_flag(sk, SOCK_USER_DATA_SET)) > + synchronize_net(); > + } > + rcu_assign_pointer(__sk_user_data((sk)), ptr); > + if (ptr) > + sock_set_flag(sk, SOCK_USER_DATA_SET); > +} > #endif /* _SOCK_H */ > > > > > There were some locking rules defined for setting ->sk_user_data: > > https://lore.kernel.org/netdev/20180124203541.3172-3-tom@quantonium.net/ > > Converting all users to either avoid using ->sk_user_data or using it > > under the proper pre-conditions has been on my TODO list for a while... > > I'll try to revive this effort. > > Avoiding using sk_user_data seems not practical. > > However making sure it is not blindly overwritten by a buggy module is doable. > At least for those that use rcu_assign_sk_user_data(). And even then, we'd need to ensure callers do properly validate the socket, to avoid overriding ->sk_user_data with a different pointer type than what other users expect. > > > > > Fixes: d00fa9adc528 ("il2tp: fix races with tunnel socket close") > > ^ > > Spurious "i". Vi user? :) > > I am not a vi user ;) > So nice to hear :) > > > > > Signed-off-by: Eric Dumazet > > > Cc: James Chapman > > > --- > > > net/l2tp/l2tp_core.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c > > > index fed6becc5daf86afa2ad9188bb28e151244bb5a6..aee33d1320184e411dbedff72b5bf5199481e53f 100644 > > > --- a/net/l2tp/l2tp_core.c > > > +++ b/net/l2tp/l2tp_core.c > > > @@ -909,7 +909,7 @@ int l2tp_udp_encap_recv(struct sock *sk, struct sk_buff *skb) > > > { > > > struct l2tp_tunnel *tunnel; > > > > > > - tunnel = l2tp_tunnel(sk); > > > + tunnel = rcu_dereference_sk_user_data(sk); > > > if (tunnel == NULL) > > > goto pass_up; > > > > > > -- > > > 2.21.0.593.g511ec345e18-goog > > >