lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Wed, 10 Feb 2016 20:22:24 +0000
From:	Jon Maloy <jon.maloy@...csson.com>
To:	Jon Maloy <jon.maloy@...csson.com>, jason <huzhijiang@...il.com>,
	"Richard Alpe" <richard.alpe@...csson.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"tipc-discussion@...ts.sourceforge.net" 
	<tipc-discussion@...ts.sourceforge.net>
Subject: RE: [tipc-discussion] [PATCH net-next v2] tipc: add peer removal
	functionality

There is of course a CPU3 involved in the first scenario. Just forgot to add it to the header.

///jon


> -----Original Message-----
> From: netdev-owner@...r.kernel.org [mailto:netdev-
> owner@...r.kernel.org] On Behalf Of Jon Maloy
> Sent: Wednesday, 10 February, 2016 14:44
> To: jason; Richard Alpe
> Cc: netdev@...r.kernel.org; tipc-discussion@...ts.sourceforge.net
> Subject: RE: [tipc-discussion] [PATCH net-next v2] tipc: add peer removal
> functionality
> 
> Hi Jason,
> I believe you are right, although the real problem is not what you think it is.
> I can see at least two interesting scenarios here,  of which I believe you are
> alluding to the first one below. However, I don't think the problem is the way
> Richard is using node_stop(), but rather with node_stop() itself.
> See below.
> 
> Scenario 1:
> --------------
> 
> CPU 1                                CPU2
> ---------                             --------
> 1: node_timeout()                                                                                     //refcnt == 1
> 2:     node_get()                                                                                             //refcnt -> 2
> 3:                                       node_stop()
> 4:                                           del_timer()                                                       // expired
> timer returns 0
> 6:                                           node_put()                                                      // refcnt -> 1
> 6:                                                                          node_find()
> 7:                                                                               node_get()                   //refcnt -> 2
> 8:    node_put()                                                                                               //refcnt -> 1
> 9:                                                                                node_put()                  //refcnt -> 0
> 10:                                                                                    node_kref_release()
> 11:                                                                                       node_delete()
> 12:                                                                                             hlist_del_rcu()
> 13:                                                                                             kfree(node)
> 
> There are several possible variations on this theme, e.g., with multiple
> parallel node_find() calls, but I don't think  they will normally lead to a crash.
> Even though  node_find() sometimes may return the reference to a node
> that was meant to be deleted, it will find the node in a state where it will just
> decide to leave it again, where after the node will be deleted.
> 
> 
> The second scenario is more problematic:
> 
> Scenario 2:
> -------------
> CPU 1                                             CPU2
> ---------                                         --------
> 1: node_timeout()                                                          //refcnt == 1
> 2:                                                  node_stop()
> 3:                                                           del_timer()          // expired timer returns 0
> 4:                                                           node_put()          //refcnt -> 0
> 5:                                                                 node_kref_release()
> 6:                                                                        node_delete()
> 7:                                                                                kfree(node)
> 8:     tipc_node_readlock(node)
> 9:   BOOM!
> 
> This one explains a crash I have seen occasionally when I am deleting a huge
> number (>100) of name spaces interconnected with TIPC. I don't know why I
> have been using del_timer() instead of del_timer_sync() here, which would
> have been the  natural choice. I probably believed that our reference
> counter handling would make it safe, which it obviously isn't.
> 
> Regards
> ///jon
> 
> 
> 
> > -----Original Message-----
> > From: jason [mailto:huzhijiang@...il.com]
> > Sent: Tuesday, 26 January, 2016 02:37
> > To: Richard Alpe
> > Cc: netdev@...r.kernel.org; tipc-discussion@...ts.sourceforge.net
> > Subject: Re: [tipc-discussion] [PATCH net-next v2] tipc: add peer
> > removal functionality
> >
> > Hi,
> >
> > On Jan 12, 2016 10:09 PM, "Richard Alpe" <richard.alpe@...csson.com>
> > wrote:
> > >
> > > Add TIPC_NL_PEER_REMOVE netlink command. This command can
> remove
> > an
> > > offline peer node from the internal data structures.
> > >
> > > This will be supported by the tipc user space tool in iproute2.
> > >
> > > Signed-off-by: Richard Alpe <richard.alpe@...csson.com>
> > > Reviewed-by: Jon Maloy <jon.maloy@...csson.com>
> > > Acked-by: Ying Xue <ying.xue@...driver.com>
> > > ---
> > >  include/uapi/linux/tipc_netlink.h |  1 +
> > >  net/tipc/net.c                    |  4 +--
> > >  net/tipc/net.h                    |  2 ++
> > >  net/tipc/netlink.c                |  5 ++++
> > >  net/tipc/node.c                   | 60
> > +++++++++++++++++++++++++++++++++++----
> > >  net/tipc/node.h                   |  3 +-
> > >  6 files changed, 66 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/include/uapi/linux/tipc_netlink.h
> > b/include/uapi/linux/tipc_netlink.h
> > > index d4c8f14..25eb645 100644
> > > --- a/include/uapi/linux/tipc_netlink.h
> > > +++ b/include/uapi/linux/tipc_netlink.h
> > > @@ -56,6 +56,7 @@ enum {
> > >         TIPC_NL_NET_GET,
> > >         TIPC_NL_NET_SET,
> > >         TIPC_NL_NAME_TABLE_GET,
> > > +       TIPC_NL_PEER_REMOVE,
> > >
> > >         __TIPC_NL_CMD_MAX,
> > >         TIPC_NL_CMD_MAX = __TIPC_NL_CMD_MAX - 1 diff --git
> > > a/net/tipc/net.c b/net/tipc/net.c index 77bf911..042d4ce 100644
> > > --- a/net/tipc/net.c
> > > +++ b/net/tipc/net.c
> > > @@ -42,7 +42,7 @@
> > >  #include "node.h"
> > >  #include "bcast.h"
> > >
> > > -static const struct nla_policy tipc_nl_net_policy[TIPC_NLA_NET_MAX
> > > + 1]
> > = {
> > > +const struct nla_policy tipc_nl_net_policy[TIPC_NLA_NET_MAX + 1] =
> > > +{
> > >         [TIPC_NLA_NET_UNSPEC]   = { .type = NLA_UNSPEC },
> > >         [TIPC_NLA_NET_ID]       = { .type = NLA_U32 }
> > >  };
> > > @@ -139,7 +139,7 @@ void tipc_net_stop(struct net *net)
> > >                               tn->own_addr);
> > >         rtnl_lock();
> > >         tipc_bearer_stop(net);
> > > -       tipc_node_stop(net);
> > > +       tipc_node_stop_net(net);
> > >         rtnl_unlock();
> > >
> > >         pr_info("Left network mode\n"); diff --git a/net/tipc/net.h
> > > b/net/tipc/net.h index 77a7a11..c7c2549 100644
> > > --- a/net/tipc/net.h
> > > +++ b/net/tipc/net.h
> > > @@ -39,6 +39,8 @@
> > >
> > >  #include <net/genetlink.h>
> > >
> > > +extern const struct nla_policy tipc_nl_net_policy[];
> > > +
> > >  int tipc_net_start(struct net *net, u32 addr);
> > >
> > >  void tipc_net_stop(struct net *net); diff --git
> > > a/net/tipc/netlink.c b/net/tipc/netlink.c index
> > > 8975b01..9019df8 100644
> > > --- a/net/tipc/netlink.c
> > > +++ b/net/tipc/netlink.c
> > > @@ -145,6 +145,11 @@ static const struct genl_ops tipc_genl_v2_ops[] =
> {
> > >                 .cmd    = TIPC_NL_NAME_TABLE_GET,
> > >                 .dumpit = tipc_nl_name_table_dump,
> > >                 .policy = tipc_nl_policy,
> > > +       },
> > > +       {
> > > +               .cmd    = TIPC_NL_PEER_REMOVE,
> > > +               .doit   = tipc_nl_peer_rm,
> > > +               .policy = tipc_nl_policy,
> > >         }
> > >  };
> > >
> > > diff --git a/net/tipc/node.c b/net/tipc/node.c index
> > > fa97d96..988cdd9
> > > 100644
> > > --- a/net/tipc/node.c
> > > +++ b/net/tipc/node.c
> > > @@ -399,17 +399,21 @@ static void tipc_node_delete(struct tipc_node
> > *node)
> > >         kfree_rcu(node, rcu);
> > >  }
> > >
> > > -void tipc_node_stop(struct net *net)
> > > +static void tipc_node_stop(struct tipc_node *node) {
> > > +       if (del_timer(&node->timer))
> > > +               tipc_node_put(node);
> > > +       tipc_node_put(node);
> > > +}
> > > +
> > > +void tipc_node_stop_net(struct net *net)
> > >  {
> > >         struct tipc_net *tn = net_generic(net, tipc_net_id);
> > >         struct tipc_node *node, *t_node;
> > >
> > >         spin_lock_bh(&tn->node_list_lock);
> > > -       list_for_each_entry_safe(node, t_node, &tn->node_list, list) {
> > > -               if (del_timer(&node->timer))
> > > -                       tipc_node_put(node);
> > > -               tipc_node_put(node);
> > > -       }
> > > +       list_for_each_entry_safe(node, t_node, &tn->node_list, list)
> > > +               tipc_node_stop(node);
> > >         spin_unlock_bh(&tn->node_list_lock);
> > >  }
> > >
> > > @@ -1529,6 +1533,50 @@ discard:
> > >         kfree_skb(skb);
> > >  }
> > >
> > > +int tipc_nl_peer_rm(struct sk_buff *skb, struct genl_info *info) {
> > > +       int err;
> > > +       u32 addr;
> > > +       struct net *net = sock_net(skb->sk);
> > > +       struct nlattr *attrs[TIPC_NLA_NET_MAX + 1];
> > > +       struct tipc_net *tn = net_generic(net, tipc_net_id);
> > > +       struct tipc_node *peer;
> > > +
> > > +       /* We identify the peer by its net */
> > > +       if (!info->attrs[TIPC_NLA_NET])
> > > +               return -EINVAL;
> > > +
> > > +       err = nla_parse_nested(attrs, TIPC_NLA_NET_MAX,
> > > +                              info->attrs[TIPC_NLA_NET],
> > > +                              tipc_nl_net_policy);
> > > +       if (err)
> > > +               return err;
> > > +
> > > +       if (!attrs[TIPC_NLA_NET_ADDR])
> > > +               return -EINVAL;
> > > +
> > > +       addr = nla_get_u32(attrs[TIPC_NLA_NET_ADDR]);
> > > +
> > > +       spin_lock_bh(&tn->node_list_lock);
> > > +       list_for_each_entry_rcu(peer, &tn->node_list, list) {
> > > +               if (peer->addr != addr)
> > > +                       continue;
> > > +
> > > +               if (peer->state == SELF_DOWN_PEER_DOWN ||
> > > +                   peer->state == SELF_DOWN_PEER_LEAVING) {
> > > +                       tipc_node_stop(peer);
> >
> > I think here is not safe to do tipc_node_stop(). Because here we are
> > not sure that node's refcount  really can drop to zero,thus, not sure
> > tipc_node_delete() can be execute in a critical region which protected
> > by node_list_lock spin lock. This may result in tipc_create_node() to
> > return a to- be-deleted node which is not in the node list any more.
> > > +
> > > +                       spin_unlock_bh(&tn->node_list_lock);
> > > +                       return 0;
> > > +               }
> > > +               spin_unlock_bh(&tn->node_list_lock);
> > > +               return -EBUSY;
> > > +       }
> > > +       spin_unlock_bh(&tn->node_list_lock);
> > > +
> > > +       return -ENXIO;
> > > +}
> > > +
> > >  int tipc_nl_node_dump(struct sk_buff *skb, struct netlink_callback
> > > *cb)  {
> > >         int err;
> > > diff --git a/net/tipc/node.h b/net/tipc/node.h index
> > > f39d9d0..8dfb6ba
> > > 100644
> > > --- a/net/tipc/node.h
> > > +++ b/net/tipc/node.h
> > > @@ -51,7 +51,7 @@ enum {
> > >  #define TIPC_NODE_CAPABILITIES TIPC_BCAST_SYNCH  #define
> > > INVALID_BEARER_ID -1
> > >
> > > -void tipc_node_stop(struct net *net);
> > > +void tipc_node_stop_net(struct net *net);
> > >  void tipc_node_check_dest(struct net *net, u32 onode,
> > >                           struct tipc_bearer *bearer,
> > >                           u16 capabilities, u32 signature, @@ -75,5
> > > +75,6 @@ int tipc_nl_node_dump_link(struct sk_buff *skb, struct
> > netlink_callback *cb);
> > >  int tipc_nl_node_reset_link_stats(struct sk_buff *skb, struct
> > > genl_info
> > *info);
> > >  int tipc_nl_node_get_link(struct sk_buff *skb, struct genl_info
> > > *info);  int tipc_nl_node_set_link(struct sk_buff *skb, struct
> > > genl_info *info);
> > > +int tipc_nl_peer_rm(struct sk_buff *skb, struct genl_info *info);
> > >
> > >  #endif
> > > --
> > > 2.1.4
> > >
> > >
> > >
> > ----------------------------------------------------------------------
> > --------
> > > Site24x7 APM Insight: Get Deep Visibility into Application
> > > Performance APM + Mobile APM + RUM: Monitor 3 App instances at just
> > > $35/Month Monitor end-to-end web transactions and take corrective
> > > actions now Troubleshoot faster and improve end-user experience.
> Signup Now!
> > > http://pubads.g.doubleclick.net/gampad/clk?id=267308311&iu=/4140
> > > _______________________________________________
> > > tipc-discussion mailing list
> > > tipc-discussion@...ts.sourceforge.net
> > > https://lists.sourceforge.net/lists/listinfo/tipc-discussion
> > ----------------------------------------------------------------------
> > --------
> > Site24x7 APM Insight: Get Deep Visibility into Application Performance
> > APM
> > + Mobile APM + RUM: Monitor 3 App instances at just $35/Month Monitor
> > end-to-end web transactions and take corrective actions now
> > Troubleshoot faster and improve end-user experience. Signup Now!
> > http://pubads.g.doubleclick.net/gampad/clk?id=267308311&iu=/4140
> > _______________________________________________
> > tipc-discussion mailing list
> > tipc-discussion@...ts.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Powered by blists - more mailing lists