| Message ID | 1551923648-17656-1-git-send-email-suyj.fnst@cn.fujitsu.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v3] net: xfrm: Add '_rcu' tag for rcu protected pointer in netns_xfrm | expand |
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!
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
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?
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.
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?
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.
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;
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(-)