[PATCH v3] net: xfrm: Add '_rcu' tag for rcu protected pointer in netns_xfrm

[PATCH v3] net: xfrm: Add '_rcu' tag for rcu protected pointer in netns_xfrm

[Su Yanjun]

For rcu protected pointers, we'd better add '__rcu' for them.

Once added '__rcu' tag for rcu protected pointer, the sparse tool reports
warnings.

net/xfrm/xfrm_user.c:1198:39: sparse:    expected struct sock *sk
net/xfrm/xfrm_user.c:1198:39: sparse:    got struct sock [noderef] <asn:4> *nlsk
[...]

So introduce a new wrapper function of nlmsg_unicast  to handle type
conversions.

This patch also fixes a direct access of a rcu protected socket.

Fixes: be33690d8fcf("[XFRM]: Fix aevent related crash")
Signed-off-by: Su Yanjun <suyj.fnst@cn.fujitsu.com>
---
Changes from v2:
- add 'Fixes' tag and some description

 include/net/netns/xfrm.h |  2 +-
 net/xfrm/xfrm_user.c     | 30 +++++++++++++++++++++++-------
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/include/net/netns/xfrm.h b/include/net/netns/xfrm.h
index 59f45b1..d2a36fb 100644
--- a/include/net/netns/xfrm.h
+++ b/include/net/netns/xfrm.h
@@ -57,7 +57,7 @@ struct netns_xfrm {
 	struct list_head	inexact_bins;
 
 
-	struct sock		*nlsk;
+	struct sock		__rcu *nlsk;
 	struct sock		*nlsk_stash;
 
 	u32			sysctl_aevent_etime;
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index d832783..e8f23e4 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1071,6 +1071,22 @@ static inline int xfrm_nlmsg_multicast(struct net *net, struct sk_buff *skb,
 	return nlmsg_multicast(nlsk, skb, pid, group, GFP_ATOMIC);
 }
 
+/* A similar wrapper like xfrm_nlmsg_multicast checking that nlsk is still
+ * available.
+ */
+static inline int xfrm_nlmsg_unicast(struct net *net, struct sk_buff *skb,
+				     u32 pid)
+{
+	struct sock *nlsk = rcu_dereference(net->xfrm.nlsk);
+
+	if (!nlsk) {
+		kfree_skb(skb);
+		return -EPIPE;
+	}
+
+	return nlmsg_unicast(nlsk, skb, pid);
+}
+
 static inline unsigned int xfrm_spdinfo_msgsize(void)
 {
 	return NLMSG_ALIGN(4)
@@ -1195,7 +1211,7 @@ static int xfrm_get_spdinfo(struct sk_buff *skb, struct nlmsghdr *nlh,
 	err = build_spdinfo(r_skb, net, sportid, seq, *flags);
 	BUG_ON(err < 0);
 
-	return nlmsg_unicast(net->xfrm.nlsk, r_skb, sportid);
+	return xfrm_nlmsg_unicast(net, r_skb, sportid);
 }
 
 static inline unsigned int xfrm_sadinfo_msgsize(void)
@@ -1254,7 +1270,7 @@ static int xfrm_get_sadinfo(struct sk_buff *skb, struct nlmsghdr *nlh,
 	err = build_sadinfo(r_skb, net, sportid, seq, *flags);
 	BUG_ON(err < 0);
 
-	return nlmsg_unicast(net->xfrm.nlsk, r_skb, sportid);
+	return xfrm_nlmsg_unicast(net, r_skb, sportid);
 }
 
 static int xfrm_get_sa(struct sk_buff *skb, struct nlmsghdr *nlh,
@@ -1274,7 +1290,7 @@ static int xfrm_get_sa(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (IS_ERR(resp_skb)) {
 		err = PTR_ERR(resp_skb);
 	} else {
-		err = nlmsg_unicast(net->xfrm.nlsk, resp_skb, NETLINK_CB(skb).portid);
+		err = xfrm_nlmsg_unicast(net, resp_skb, NETLINK_CB(skb).portid);
 	}
 	xfrm_state_put(x);
 out_noput:
@@ -1337,7 +1353,7 @@ static int xfrm_alloc_userspi(struct sk_buff *skb, struct nlmsghdr *nlh,
 		goto out;
 	}
 
-	err = nlmsg_unicast(net->xfrm.nlsk, resp_skb, NETLINK_CB(skb).portid);
+	err = xfrm_nlmsg_unicast(net, resp_skb, NETLINK_CB(skb).portid);
 
 out:
 	xfrm_state_put(x);
@@ -1903,8 +1919,8 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
 		if (IS_ERR(resp_skb)) {
 			err = PTR_ERR(resp_skb);
 		} else {
-			err = nlmsg_unicast(net->xfrm.nlsk, resp_skb,
-					    NETLINK_CB(skb).portid);
+			err = xfrm_nlmsg_unicast(net, resp_skb,
+						 NETLINK_CB(skb).portid);
 		}
 	} else {
 		xfrm_audit_policy_delete(xp, err ? 0 : 1, true);
@@ -2062,7 +2078,7 @@ static int xfrm_get_ae(struct sk_buff *skb, struct nlmsghdr *nlh,
 	err = build_aevent(r_skb, x, &c);
 	BUG_ON(err < 0);
 
-	err = nlmsg_unicast(net->xfrm.nlsk, r_skb, NETLINK_CB(skb).portid);
+	err = xfrm_nlmsg_unicast(net, r_skb, NETLINK_CB(skb).portid);
 	spin_unlock_bh(&x->lock);
 	xfrm_state_put(x);
 	return err;
-- 
2.7.4

Re: [PATCH v3] net: xfrm: Add '_rcu' tag for rcu protected pointer in netns_xfrm

[Steffen Klassert]

On Wed, Mar 06, 2019 at 08:54:08PM -0500, Su Yanjun wrote:
> For rcu protected pointers, we'd better add '__rcu' for them.
> 
> Once added '__rcu' tag for rcu protected pointer, the sparse tool reports
> warnings.
> 
> net/xfrm/xfrm_user.c:1198:39: sparse:    expected struct sock *sk
> net/xfrm/xfrm_user.c:1198:39: sparse:    got struct sock [noderef] <asn:4> *nlsk
> [...]
> 
> So introduce a new wrapper function of nlmsg_unicast  to handle type
> conversions.
> 
> This patch also fixes a direct access of a rcu protected socket.
> 
> Fixes: be33690d8fcf("[XFRM]: Fix aevent related crash")
> Signed-off-by: Su Yanjun <suyj.fnst@cn.fujitsu.com>

Patch applied, thanks!

Re: [PATCH v3] net: xfrm: Add '_rcu' tag for rcu protected pointer in netns_xfrm

[Eric Dumazet]

On 03/11/2019 03:10 AM, Steffen Klassert wrote:
> On Wed, Mar 06, 2019 at 08:54:08PM -0500, Su Yanjun wrote:
>> For rcu protected pointers, we'd better add '__rcu' for them.
>>
>> Once added '__rcu' tag for rcu protected pointer, the sparse tool reports
>> warnings.
>>
>> net/xfrm/xfrm_user.c:1198:39: sparse:    expected struct sock *sk
>> net/xfrm/xfrm_user.c:1198:39: sparse:    got struct sock [noderef] <asn:4> *nlsk
>> [...]
>>
>> So introduce a new wrapper function of nlmsg_unicast  to handle type
>> conversions.
>>
>> This patch also fixes a direct access of a rcu protected socket.
>>
>> Fixes: be33690d8fcf("[XFRM]: Fix aevent related crash")
>> Signed-off-by: Su Yanjun <suyj.fnst@cn.fujitsu.com>
> 
> Patch applied, thanks!
> 

Has this patch ever been tested ?

I do not see the needed rcu_read_lock() / rcu_read_unlock() that are mandated for
rcu_dereference() correctness.

All changes like that should be tested with :

CONFIG_LOCKDEP=y
CONFIG_PROVE_RCU=y

Re: [PATCH v3] net: xfrm: Add '_rcu' tag for rcu protected pointer in netns_xfrm

[Steffen Klassert]

On Mon, Mar 18, 2019 at 10:22:46AM -0700, Eric Dumazet wrote:
> 
> 
> On 03/11/2019 03:10 AM, Steffen Klassert wrote:
> > On Wed, Mar 06, 2019 at 08:54:08PM -0500, Su Yanjun wrote:
> >> For rcu protected pointers, we'd better add '__rcu' for them.
> >>
> >> Once added '__rcu' tag for rcu protected pointer, the sparse tool reports
> >> warnings.
> >>
> >> net/xfrm/xfrm_user.c:1198:39: sparse:    expected struct sock *sk
> >> net/xfrm/xfrm_user.c:1198:39: sparse:    got struct sock [noderef] <asn:4> *nlsk
> >> [...]
> >>
> >> So introduce a new wrapper function of nlmsg_unicast  to handle type
> >> conversions.
> >>
> >> This patch also fixes a direct access of a rcu protected socket.
> >>
> >> Fixes: be33690d8fcf("[XFRM]: Fix aevent related crash")
> >> Signed-off-by: Su Yanjun <suyj.fnst@cn.fujitsu.com>
> > 
> > Patch applied, thanks!
> > 
> 
> Has this patch ever been tested ?

I had this on your testing system and it did
not complain. But maybe my testcases did not
trigger that codepath. Su, can you answer
on Eric question?

Re: [PATCH v3] net: xfrm: Add '_rcu' tag for rcu protected pointer in netns_xfrm

[Eric Dumazet]

On 03/19/2019 08:15 AM, Steffen Klassert wrote:
> On Mon, Mar 18, 2019 at 10:22:46AM -0700, Eric Dumazet wrote:
>>
>>
>> On 03/11/2019 03:10 AM, Steffen Klassert wrote:
>>> On Wed, Mar 06, 2019 at 08:54:08PM -0500, Su Yanjun wrote:
>>>> For rcu protected pointers, we'd better add '__rcu' for them.
>>>>
>>>> Once added '__rcu' tag for rcu protected pointer, the sparse tool reports
>>>> warnings.
>>>>
>>>> net/xfrm/xfrm_user.c:1198:39: sparse:    expected struct sock *sk
>>>> net/xfrm/xfrm_user.c:1198:39: sparse:    got struct sock [noderef] <asn:4> *nlsk
>>>> [...]
>>>>
>>>> So introduce a new wrapper function of nlmsg_unicast  to handle type
>>>> conversions.
>>>>
>>>> This patch also fixes a direct access of a rcu protected socket.
>>>>
>>>> Fixes: be33690d8fcf("[XFRM]: Fix aevent related crash")
>>>> Signed-off-by: Su Yanjun <suyj.fnst@cn.fujitsu.com>
>>>
>>> Patch applied, thanks!
>>>
>>
>> Has this patch ever been tested ?
> 
> I had this on your testing system and it did
> not complain. But maybe my testcases did not
> trigger that codepath. Su, can you answer
> on Eric question?
> 

I can release four syzbot reports if you want.

Re: [PATCH v3] net: xfrm: Add '_rcu' tag for rcu protected pointer in netns_xfrm

[Su Yanjun <suyj.fnst@cn.fujitsu.com>]

On 2019/3/19 23:15, Steffen Klassert wrote:
> On Mon, Mar 18, 2019 at 10:22:46AM -0700, Eric Dumazet wrote:
>>
>> On 03/11/2019 03:10 AM, Steffen Klassert wrote:
>>> On Wed, Mar 06, 2019 at 08:54:08PM -0500, Su Yanjun wrote:
>>>> For rcu protected pointers, we'd better add '__rcu' for them.
>>>>
>>>> Once added '__rcu' tag for rcu protected pointer, the sparse tool reports
>>>> warnings.
>>>>
>>>> net/xfrm/xfrm_user.c:1198:39: sparse:    expected struct sock *sk
>>>> net/xfrm/xfrm_user.c:1198:39: sparse:    got struct sock [noderef] <asn:4> *nlsk
>>>> [...]
>>>>
>>>> So introduce a new wrapper function of nlmsg_unicast  to handle type
>>>> conversions.
>>>>
>>>> This patch also fixes a direct access of a rcu protected socket.
>>>>
>>>> Fixes: be33690d8fcf("[XFRM]: Fix aevent related crash")
>>>> Signed-off-by: Su Yanjun <suyj.fnst@cn.fujitsu.com>
>>> Patch applied, thanks!
>>>
>> Has this patch ever been tested ?
> I had this on your testing system and it did
> not complain. But maybe my testcases did not
> trigger that codepath. Su, can you answer
> on Eric question?
>
Firs of all, I didn't produce it on my test machine.

Maybe we need recompile the kernel with Eric Dumazet's advise.

CONFIG_LOCKDEP=y
CONFIG_PROVE_RCU=y

The second the code path indeed doesn't do as below:
rcu_read_lock()
rcu_dereference()
...
rcu_read_unlock()

All rcu_dereference are in the follow code path:
xfrm_user_rcv_msg
  link->doit(skb, nlh, attrs)
     rcu_dereference()

I think we can add rcu protection for nlsock

xfrm_user_rcv_msg
   rcu_read_lock()
   link->doit(skb, nlh, attrs)
   rcu_read_unlock()

Any advise?

Re: [PATCH v3] net: xfrm: Add '_rcu' tag for rcu protected pointer in netns_xfrm

[Steffen Klassert]

On Wed, Mar 20, 2019 at 08:53:54AM +0800, Su Yanjun <suyj.fnst@cn.fujitsu.com> wrote:
> 
> Firs of all, I didn't produce it on my test machine.

First of all, you should provide better quality patches.

All of your recent patches had issues, some were sorted 
out before applying other not. Bugs happen, but it is
not acceptable that all your patches are buggy.

I've reverted this patch like Eric suggested.