* [0/2] Fix problems with with IFLA_VF_PORTS
@ 2014-04-23 7:21 David Gibson
2014-04-23 7:21 ` [PATCH 1/2] rtnetlink: Warn when interface's information won't fit in our packet David Gibson
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: David Gibson @ 2014-04-23 7:21 UTC (permalink / raw)
To: netdev
Cc: ssujith, neepatel, benve, davem, ben, govindarajulu90, gregory.v.rose
I've had a customer encounter a problem with getifaddrs(3) freezing up
on a system with a Cisco enic device.
I've discovered that the problem is caused by an enic device with a
large number of SR-IOV virtual functions overflowing the normal sized
packet buffer for netlink, leading to interfaces not being reported
from an RTM_GETLINK request.
The first patch here just makes the problem easier to locate if it
occurs again in a different way, by adding a WARN_ON() when we run out
of room in a netlink packet in this manner.
The second patch actually fixes the problem, by only reporting
IFLA_VF_PORTS information when the RTEXT_FILTER_VF flag is specified.
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/2] rtnetlink: Warn when interface's information won't fit in our packet 2014-04-23 7:21 [0/2] Fix problems with with IFLA_VF_PORTS David Gibson @ 2014-04-23 7:21 ` David Gibson 2014-04-23 9:09 ` Jiri Pirko 2014-04-23 14:20 ` Sergei Shtylyov 2014-04-23 7:21 ` [PATCH 2/2] rtnetlink: Only supply IFLA_VF_PORTS information when RTEXT_FILTER_VF is set David Gibson 2014-04-23 15:14 ` [0/2] Fix problems with with IFLA_VF_PORTS Greg Rose 2 siblings, 2 replies; 12+ messages in thread From: David Gibson @ 2014-04-23 7:21 UTC (permalink / raw) To: netdev Cc: ssujith, neepatel, benve, davem, ben, govindarajulu90, gregory.v.rose, David Gibson Without IFLA_EXT_MASK specified, the information reported for a single interface in response to RTM_GETLINK is expected to fit within a netlink packet of NLMSG_GOODSIZE. If it doesn't, however, things will go badly wrong, When listing all interfaces, netlink_dump() will incorrectly treat -EMSGSIZE on the first message in a packet as the end of the listing and omit information for that interface and all subsequent ones. This can cause getifaddrs(3) to enter an infinite loop. This patch won't fix the problem, but it will WARN_ON() making it easier to track down what's going wrong. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- net/core/rtnetlink.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index d4ff417..5331db2 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -1198,6 +1198,7 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) struct hlist_head *head; struct nlattr *tb[IFLA_MAX+1]; u32 ext_filter_mask = 0; + int err; s_h = cb->args[0]; s_idx = cb->args[1]; @@ -1218,11 +1219,16 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) hlist_for_each_entry_rcu(dev, head, index_hlist) { if (idx < s_idx) goto cont; - if (rtnl_fill_ifinfo(skb, dev, RTM_NEWLINK, - NETLINK_CB(cb->skb).portid, - cb->nlh->nlmsg_seq, 0, - NLM_F_MULTI, - ext_filter_mask) <= 0) + err = rtnl_fill_ifinfo(skb, dev, RTM_NEWLINK, + NETLINK_CB(cb->skb).portid, + cb->nlh->nlmsg_seq, 0, + NLM_F_MULTI, + ext_filter_mask); + /* If we ran out of room on the first message, + * we're in trouble */ + WARN_ON((err == -EMSGSIZE) && (skb->len == 0)); + + if (err <= 0) goto out; nl_dump_check_consistent(cb, nlmsg_hdr(skb)); -- 1.9.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] rtnetlink: Warn when interface's information won't fit in our packet 2014-04-23 7:21 ` [PATCH 1/2] rtnetlink: Warn when interface's information won't fit in our packet David Gibson @ 2014-04-23 9:09 ` Jiri Pirko 2014-04-23 11:41 ` David Gibson 2014-04-23 14:20 ` Sergei Shtylyov 1 sibling, 1 reply; 12+ messages in thread From: Jiri Pirko @ 2014-04-23 9:09 UTC (permalink / raw) To: David Gibson Cc: netdev, ssujith, neepatel, benve, davem, ben, govindarajulu90, gregory.v.rose Wed, Apr 23, 2014 at 09:21:04AM CEST, david@gibson.dropbear.id.au wrote: >Without IFLA_EXT_MASK specified, the information reported for a single >interface in response to RTM_GETLINK is expected to fit within a netlink >packet of NLMSG_GOODSIZE. > >If it doesn't, however, things will go badly wrong, When listing all >interfaces, netlink_dump() will incorrectly treat -EMSGSIZE on the first >message in a packet as the end of the listing and omit information for >that interface and all subsequent ones. This can cause getifaddrs(3) to >enter an infinite loop. > >This patch won't fix the problem, but it will WARN_ON() making it easier to >track down what's going wrong. > >Signed-off-by: David Gibson <david@gibson.dropbear.id.au> >--- > net/core/rtnetlink.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > >diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c >index d4ff417..5331db2 100644 >--- a/net/core/rtnetlink.c >+++ b/net/core/rtnetlink.c >@@ -1198,6 +1198,7 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) > struct hlist_head *head; > struct nlattr *tb[IFLA_MAX+1]; > u32 ext_filter_mask = 0; >+ int err; > > s_h = cb->args[0]; > s_idx = cb->args[1]; >@@ -1218,11 +1219,16 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) > hlist_for_each_entry_rcu(dev, head, index_hlist) { > if (idx < s_idx) > goto cont; >- if (rtnl_fill_ifinfo(skb, dev, RTM_NEWLINK, >- NETLINK_CB(cb->skb).portid, >- cb->nlh->nlmsg_seq, 0, >- NLM_F_MULTI, >- ext_filter_mask) <= 0) >+ err = rtnl_fill_ifinfo(skb, dev, RTM_NEWLINK, >+ NETLINK_CB(cb->skb).portid, >+ cb->nlh->nlmsg_seq, 0, >+ NLM_F_MULTI, >+ ext_filter_mask); >+ /* If we ran out of room on the first message, >+ * we're in trouble */ >+ WARN_ON((err == -EMSGSIZE) && (skb->len == 0)); >+ >+ if (err <= 0) if (err) > goto out; > > nl_dump_check_consistent(cb, nlmsg_hdr(skb)); >-- >1.9.0 > >-- >To unsubscribe from this list: send the line "unsubscribe netdev" in >the body of a message to majordomo@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] rtnetlink: Warn when interface's information won't fit in our packet 2014-04-23 9:09 ` Jiri Pirko @ 2014-04-23 11:41 ` David Gibson 2014-04-23 11:51 ` Jiri Pirko 0 siblings, 1 reply; 12+ messages in thread From: David Gibson @ 2014-04-23 11:41 UTC (permalink / raw) To: Jiri Pirko Cc: netdev, ssujith, neepatel, benve, davem, ben, govindarajulu90, gregory.v.rose [-- Attachment #1: Type: text/plain, Size: 2475 bytes --] On Wed, Apr 23, 2014 at 11:09:05AM +0200, Jiri Pirko wrote: > Wed, Apr 23, 2014 at 09:21:04AM CEST, david@gibson.dropbear.id.au wrote: > >Without IFLA_EXT_MASK specified, the information reported for a single > >interface in response to RTM_GETLINK is expected to fit within a netlink > >packet of NLMSG_GOODSIZE. > > > >If it doesn't, however, things will go badly wrong, When listing all > >interfaces, netlink_dump() will incorrectly treat -EMSGSIZE on the first > >message in a packet as the end of the listing and omit information for > >that interface and all subsequent ones. This can cause getifaddrs(3) to > >enter an infinite loop. > > > >This patch won't fix the problem, but it will WARN_ON() making it easier to > >track down what's going wrong. > > > >Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > >--- > > net/core/rtnetlink.c | 16 +++++++++++----- > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > >diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > >index d4ff417..5331db2 100644 > >--- a/net/core/rtnetlink.c > >+++ b/net/core/rtnetlink.c > >@@ -1198,6 +1198,7 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) > > struct hlist_head *head; > > struct nlattr *tb[IFLA_MAX+1]; > > u32 ext_filter_mask = 0; > >+ int err; > > > > s_h = cb->args[0]; > > s_idx = cb->args[1]; > >@@ -1218,11 +1219,16 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) > > hlist_for_each_entry_rcu(dev, head, index_hlist) { > > if (idx < s_idx) > > goto cont; > >- if (rtnl_fill_ifinfo(skb, dev, RTM_NEWLINK, > >- NETLINK_CB(cb->skb).portid, > >- cb->nlh->nlmsg_seq, 0, > >- NLM_F_MULTI, > >- ext_filter_mask) <= 0) > >+ err = rtnl_fill_ifinfo(skb, dev, RTM_NEWLINK, > >+ NETLINK_CB(cb->skb).portid, > >+ cb->nlh->nlmsg_seq, 0, > >+ NLM_F_MULTI, > >+ ext_filter_mask); > >+ /* If we ran out of room on the first message, > >+ * we're in trouble */ > >+ WARN_ON((err == -EMSGSIZE) && (skb->len == 0)); > >+ > >+ if (err <= 0) > if (err) <= 0 was what the original version had. Why do you think it should be changed to != 0? -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] rtnetlink: Warn when interface's information won't fit in our packet 2014-04-23 11:41 ` David Gibson @ 2014-04-23 11:51 ` Jiri Pirko 0 siblings, 0 replies; 12+ messages in thread From: Jiri Pirko @ 2014-04-23 11:51 UTC (permalink / raw) To: David Gibson Cc: netdev, ssujith, neepatel, benve, davem, ben, govindarajulu90, gregory.v.rose Wed, Apr 23, 2014 at 01:41:20PM CEST, david@gibson.dropbear.id.au wrote: >On Wed, Apr 23, 2014 at 11:09:05AM +0200, Jiri Pirko wrote: >> Wed, Apr 23, 2014 at 09:21:04AM CEST, david@gibson.dropbear.id.au wrote: >> >Without IFLA_EXT_MASK specified, the information reported for a single >> >interface in response to RTM_GETLINK is expected to fit within a netlink >> >packet of NLMSG_GOODSIZE. >> > >> >If it doesn't, however, things will go badly wrong, When listing all >> >interfaces, netlink_dump() will incorrectly treat -EMSGSIZE on the first >> >message in a packet as the end of the listing and omit information for >> >that interface and all subsequent ones. This can cause getifaddrs(3) to >> >enter an infinite loop. >> > >> >This patch won't fix the problem, but it will WARN_ON() making it easier to >> >track down what's going wrong. >> > >> >Signed-off-by: David Gibson <david@gibson.dropbear.id.au> >> >--- >> > net/core/rtnetlink.c | 16 +++++++++++----- >> > 1 file changed, 11 insertions(+), 5 deletions(-) >> > >> >diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c >> >index d4ff417..5331db2 100644 >> >--- a/net/core/rtnetlink.c >> >+++ b/net/core/rtnetlink.c >> >@@ -1198,6 +1198,7 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) >> > struct hlist_head *head; >> > struct nlattr *tb[IFLA_MAX+1]; >> > u32 ext_filter_mask = 0; >> >+ int err; >> > >> > s_h = cb->args[0]; >> > s_idx = cb->args[1]; >> >@@ -1218,11 +1219,16 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) >> > hlist_for_each_entry_rcu(dev, head, index_hlist) { >> > if (idx < s_idx) >> > goto cont; >> >- if (rtnl_fill_ifinfo(skb, dev, RTM_NEWLINK, >> >- NETLINK_CB(cb->skb).portid, >> >- cb->nlh->nlmsg_seq, 0, >> >- NLM_F_MULTI, >> >- ext_filter_mask) <= 0) >> >+ err = rtnl_fill_ifinfo(skb, dev, RTM_NEWLINK, >> >+ NETLINK_CB(cb->skb).portid, >> >+ cb->nlh->nlmsg_seq, 0, >> >+ NLM_F_MULTI, >> >+ ext_filter_mask); >> >+ /* If we ran out of room on the first message, >> >+ * we're in trouble */ >> >+ WARN_ON((err == -EMSGSIZE) && (skb->len == 0)); >> >+ >> >+ if (err <= 0) >> if (err) > ><= 0 was what the original version had. Why do you think it should be >changed to != 0? You are right. I misread rtnl_fill_ifinfo. > >-- >David Gibson | I'll have my music baroque, and my code >david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! >http://www.ozlabs.org/~dgibson ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] rtnetlink: Warn when interface's information won't fit in our packet 2014-04-23 7:21 ` [PATCH 1/2] rtnetlink: Warn when interface's information won't fit in our packet David Gibson 2014-04-23 9:09 ` Jiri Pirko @ 2014-04-23 14:20 ` Sergei Shtylyov 1 sibling, 0 replies; 12+ messages in thread From: Sergei Shtylyov @ 2014-04-23 14:20 UTC (permalink / raw) To: David Gibson, netdev Cc: ssujith, neepatel, benve, davem, ben, govindarajulu90, gregory.v.rose Hello. On 23-04-2014 11:21, David Gibson wrote: > Without IFLA_EXT_MASK specified, the information reported for a single > interface in response to RTM_GETLINK is expected to fit within a netlink > packet of NLMSG_GOODSIZE. > If it doesn't, however, things will go badly wrong, When listing all > interfaces, netlink_dump() will incorrectly treat -EMSGSIZE on the first > message in a packet as the end of the listing and omit information for > that interface and all subsequent ones. This can cause getifaddrs(3) to > enter an infinite loop. > This patch won't fix the problem, but it will WARN_ON() making it easier to > track down what's going wrong. > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > net/core/rtnetlink.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index d4ff417..5331db2 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c [...] > @@ -1218,11 +1219,16 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) > hlist_for_each_entry_rcu(dev, head, index_hlist) { > if (idx < s_idx) > goto cont; > - if (rtnl_fill_ifinfo(skb, dev, RTM_NEWLINK, > - NETLINK_CB(cb->skb).portid, > - cb->nlh->nlmsg_seq, 0, > - NLM_F_MULTI, > - ext_filter_mask) <= 0) > + err = rtnl_fill_ifinfo(skb, dev, RTM_NEWLINK, > + NETLINK_CB(cb->skb).portid, > + cb->nlh->nlmsg_seq, 0, > + NLM_F_MULTI, > + ext_filter_mask); > + /* If we ran out of room on the first message, > + * we're in trouble */ Hm, comment style... multi-line comments in the networking code should look like this: /* bla * bla */ > + WARN_ON((err == -EMSGSIZE) && (skb->len == 0)); > + > + if (err <= 0) WBR, Sergei ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/2] rtnetlink: Only supply IFLA_VF_PORTS information when RTEXT_FILTER_VF is set 2014-04-23 7:21 [0/2] Fix problems with with IFLA_VF_PORTS David Gibson 2014-04-23 7:21 ` [PATCH 1/2] rtnetlink: Warn when interface's information won't fit in our packet David Gibson @ 2014-04-23 7:21 ` David Gibson 2014-04-23 9:17 ` Jiri Pirko 2014-04-23 15:14 ` [0/2] Fix problems with with IFLA_VF_PORTS Greg Rose 2 siblings, 1 reply; 12+ messages in thread From: David Gibson @ 2014-04-23 7:21 UTC (permalink / raw) To: netdev Cc: ssujith, neepatel, benve, davem, ben, govindarajulu90, gregory.v.rose, David Gibson Since 115c9b81928360d769a76c632bae62d15206a94a (rtnetlink: Fix problem with buffer allocation), RTM_NEWLINK messages only contain the IFLA_VFINFO_LIST attribute if they were solicited by a GETLINK message containing an IFLA_EXT_MASK attribute with the RTEXT_FILTER_VF flag. That was done because some user programs broke when they received more data than expected - because IFLA_VFINFO_LIST contains information for each VF it can become large if there are many VFs. However, the IFLA_VF_PORTS attribute, supplied for devices which implement ndo_get_vf_port (currently the 'enic' driver only), has the same problem. It supplies per-VF information and can therefore become large, but it is not currently conditional on the IFLA_EXT_MASK value. Worse, it interacts badly with the existing EXT_MASK handling. When IFLA_EXT_MASK is not supplied, the buffer for netlink replies is fixed at NLMSG_GOODSIZE. If the information for IFLA_VF_PORTS exceeds this, then rtnl_fill_ifinfo() returns -EMSGSIZE on the first message in a packet. netlink_dump() will misinterpret this as having finished the listing and omit data for this interface and all subsequent ones. That can cause getifaddrs(3) to enter an infinite loop. This patch addresses the problem by only supplying IFLA_VF_PORTS when IFLA_EXT_MASK is supplied with the RTEXT_FILTER_VF flag set. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- net/core/rtnetlink.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 5331db2..32a1287 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -774,7 +774,8 @@ static inline int rtnl_vfinfo_size(const struct net_device *dev, return 0; } -static size_t rtnl_port_size(const struct net_device *dev) +static size_t rtnl_port_size(const struct net_device *dev, + u32 ext_filter_mask) { size_t port_size = nla_total_size(4) /* PORT_VF */ + nla_total_size(PORT_PROFILE_MAX) /* PORT_PROFILE */ @@ -790,7 +791,8 @@ static size_t rtnl_port_size(const struct net_device *dev) size_t port_self_size = nla_total_size(sizeof(struct nlattr)) + port_size; - if (!dev->netdev_ops->ndo_get_vf_port || !dev->dev.parent) + if (!dev->netdev_ops->ndo_get_vf_port || !dev->dev.parent + || !(ext_filter_mask & RTEXT_FILTER_VF)) return 0; if (dev_num_vf(dev->dev.parent)) return port_self_size + vf_ports_size + @@ -826,7 +828,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev, + nla_total_size(ext_filter_mask & RTEXT_FILTER_VF ? 4 : 0) /* IFLA_NUM_VF */ + rtnl_vfinfo_size(dev, ext_filter_mask) /* IFLA_VFINFO_LIST */ - + rtnl_port_size(dev) /* IFLA_VF_PORTS + IFLA_PORT_SELF */ + + rtnl_port_size(dev, ext_filter_mask) /* IFLA_VF_PORTS + IFLA_PORT_SELF */ + rtnl_link_get_size(dev) /* IFLA_LINKINFO */ + rtnl_link_get_af_size(dev) /* IFLA_AF_SPEC */ + nla_total_size(MAX_PHYS_PORT_ID_LEN); /* IFLA_PHYS_PORT_ID */ @@ -888,11 +890,13 @@ static int rtnl_port_self_fill(struct sk_buff *skb, struct net_device *dev) return 0; } -static int rtnl_port_fill(struct sk_buff *skb, struct net_device *dev) +static int rtnl_port_fill(struct sk_buff *skb, struct net_device *dev, + u32 ext_filter_mask) { int err; - if (!dev->netdev_ops->ndo_get_vf_port || !dev->dev.parent) + if (!dev->netdev_ops->ndo_get_vf_port || !dev->dev.parent + || !(ext_filter_mask & RTEXT_FILTER_VF)) return 0; err = rtnl_port_self_fill(skb, dev); @@ -1079,7 +1083,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev, nla_nest_end(skb, vfinfo); } - if (rtnl_port_fill(skb, dev)) + if (rtnl_port_fill(skb, dev, ext_filter_mask)) goto nla_put_failure; if (dev->rtnl_link_ops || rtnl_have_link_slave_info(dev)) { -- 1.9.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] rtnetlink: Only supply IFLA_VF_PORTS information when RTEXT_FILTER_VF is set 2014-04-23 7:21 ` [PATCH 2/2] rtnetlink: Only supply IFLA_VF_PORTS information when RTEXT_FILTER_VF is set David Gibson @ 2014-04-23 9:17 ` Jiri Pirko 2014-04-24 0:13 ` David Gibson 0 siblings, 1 reply; 12+ messages in thread From: Jiri Pirko @ 2014-04-23 9:17 UTC (permalink / raw) To: David Gibson Cc: netdev, ssujith, neepatel, benve, davem, ben, govindarajulu90, gregory.v.rose Wed, Apr 23, 2014 at 09:21:05AM CEST, david@gibson.dropbear.id.au wrote: >Since 115c9b81928360d769a76c632bae62d15206a94a (rtnetlink: Fix problem with >buffer allocation), RTM_NEWLINK messages only contain the IFLA_VFINFO_LIST >attribute if they were solicited by a GETLINK message containing an >IFLA_EXT_MASK attribute with the RTEXT_FILTER_VF flag. > >That was done because some user programs broke when they received more data >than expected - because IFLA_VFINFO_LIST contains information for each VF >it can become large if there are many VFs. > >However, the IFLA_VF_PORTS attribute, supplied for devices which implement >ndo_get_vf_port (currently the 'enic' driver only), has the same problem. >It supplies per-VF information and can therefore become large, but it is >not currently conditional on the IFLA_EXT_MASK value. > >Worse, it interacts badly with the existing EXT_MASK handling. When >IFLA_EXT_MASK is not supplied, the buffer for netlink replies is fixed at >NLMSG_GOODSIZE. If the information for IFLA_VF_PORTS exceeds this, then >rtnl_fill_ifinfo() returns -EMSGSIZE on the first message in a packet. >netlink_dump() will misinterpret this as having finished the listing and >omit data for this interface and all subsequent ones. That can cause >getifaddrs(3) to enter an infinite loop. > >This patch addresses the problem by only supplying IFLA_VF_PORTS when >IFLA_EXT_MASK is supplied with the RTEXT_FILTER_VF flag set. > >Signed-off-by: David Gibson <david@gibson.dropbear.id.au> >--- > net/core/rtnetlink.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > >diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c >index 5331db2..32a1287 100644 >--- a/net/core/rtnetlink.c >+++ b/net/core/rtnetlink.c >@@ -774,7 +774,8 @@ static inline int rtnl_vfinfo_size(const struct net_device *dev, > return 0; > } > >-static size_t rtnl_port_size(const struct net_device *dev) >+static size_t rtnl_port_size(const struct net_device *dev, >+ u32 ext_filter_mask) > { > size_t port_size = nla_total_size(4) /* PORT_VF */ > + nla_total_size(PORT_PROFILE_MAX) /* PORT_PROFILE */ >@@ -790,7 +791,8 @@ static size_t rtnl_port_size(const struct net_device *dev) > size_t port_self_size = nla_total_size(sizeof(struct nlattr)) > + port_size; > >- if (!dev->netdev_ops->ndo_get_vf_port || !dev->dev.parent) >+ if (!dev->netdev_ops->ndo_get_vf_port || !dev->dev.parent >+ || !(ext_filter_mask & RTEXT_FILTER_VF)) Do not start line with || > return 0; > if (dev_num_vf(dev->dev.parent)) > return port_self_size + vf_ports_size + >@@ -826,7 +828,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev, > + nla_total_size(ext_filter_mask > & RTEXT_FILTER_VF ? 4 : 0) /* IFLA_NUM_VF */ > + rtnl_vfinfo_size(dev, ext_filter_mask) /* IFLA_VFINFO_LIST */ >- + rtnl_port_size(dev) /* IFLA_VF_PORTS + IFLA_PORT_SELF */ >+ + rtnl_port_size(dev, ext_filter_mask) /* IFLA_VF_PORTS + IFLA_PORT_SELF */ > + rtnl_link_get_size(dev) /* IFLA_LINKINFO */ > + rtnl_link_get_af_size(dev) /* IFLA_AF_SPEC */ > + nla_total_size(MAX_PHYS_PORT_ID_LEN); /* IFLA_PHYS_PORT_ID */ >@@ -888,11 +890,13 @@ static int rtnl_port_self_fill(struct sk_buff *skb, struct net_device *dev) > return 0; > } > >-static int rtnl_port_fill(struct sk_buff *skb, struct net_device *dev) >+static int rtnl_port_fill(struct sk_buff *skb, struct net_device *dev, >+ u32 ext_filter_mask) > { > int err; > >- if (!dev->netdev_ops->ndo_get_vf_port || !dev->dev.parent) >+ if (!dev->netdev_ops->ndo_get_vf_port || !dev->dev.parent >+ || !(ext_filter_mask & RTEXT_FILTER_VF)) Same here. > return 0; > > err = rtnl_port_self_fill(skb, dev); >@@ -1079,7 +1083,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev, > nla_nest_end(skb, vfinfo); > } > >- if (rtnl_port_fill(skb, dev)) >+ if (rtnl_port_fill(skb, dev, ext_filter_mask)) > goto nla_put_failure; > > if (dev->rtnl_link_ops || rtnl_have_link_slave_info(dev)) { Other than this, I think the patch is fine. >-- >1.9.0 > >-- >To unsubscribe from this list: send the line "unsubscribe netdev" in >the body of a message to majordomo@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] rtnetlink: Only supply IFLA_VF_PORTS information when RTEXT_FILTER_VF is set 2014-04-23 9:17 ` Jiri Pirko @ 2014-04-24 0:13 ` David Gibson 0 siblings, 0 replies; 12+ messages in thread From: David Gibson @ 2014-04-24 0:13 UTC (permalink / raw) To: Jiri Pirko Cc: netdev, ssujith, neepatel, benve, davem, ben, govindarajulu90, gregory.v.rose [-- Attachment #1: Type: text/plain, Size: 2986 bytes --] On Wed, Apr 23, 2014 at 11:17:15AM +0200, Jiri Pirko wrote: > Wed, Apr 23, 2014 at 09:21:05AM CEST, david@gibson.dropbear.id.au wrote: > >Since 115c9b81928360d769a76c632bae62d15206a94a (rtnetlink: Fix problem with > >buffer allocation), RTM_NEWLINK messages only contain the IFLA_VFINFO_LIST > >attribute if they were solicited by a GETLINK message containing an > >IFLA_EXT_MASK attribute with the RTEXT_FILTER_VF flag. > > > >That was done because some user programs broke when they received more data > >than expected - because IFLA_VFINFO_LIST contains information for each VF > >it can become large if there are many VFs. > > > >However, the IFLA_VF_PORTS attribute, supplied for devices which implement > >ndo_get_vf_port (currently the 'enic' driver only), has the same problem. > >It supplies per-VF information and can therefore become large, but it is > >not currently conditional on the IFLA_EXT_MASK value. > > > >Worse, it interacts badly with the existing EXT_MASK handling. When > >IFLA_EXT_MASK is not supplied, the buffer for netlink replies is fixed at > >NLMSG_GOODSIZE. If the information for IFLA_VF_PORTS exceeds this, then > >rtnl_fill_ifinfo() returns -EMSGSIZE on the first message in a packet. > >netlink_dump() will misinterpret this as having finished the listing and > >omit data for this interface and all subsequent ones. That can cause > >getifaddrs(3) to enter an infinite loop. > > > >This patch addresses the problem by only supplying IFLA_VF_PORTS when > >IFLA_EXT_MASK is supplied with the RTEXT_FILTER_VF flag set. > > > >Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > >--- > > net/core/rtnetlink.c | 16 ++++++++++------ > > 1 file changed, 10 insertions(+), 6 deletions(-) > > > >diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > >index 5331db2..32a1287 100644 > >--- a/net/core/rtnetlink.c > >+++ b/net/core/rtnetlink.c > >@@ -774,7 +774,8 @@ static inline int rtnl_vfinfo_size(const struct net_device *dev, > > return 0; > > } > > > >-static size_t rtnl_port_size(const struct net_device *dev) > >+static size_t rtnl_port_size(const struct net_device *dev, > >+ u32 ext_filter_mask) > > { > > size_t port_size = nla_total_size(4) /* PORT_VF */ > > + nla_total_size(PORT_PROFILE_MAX) /* PORT_PROFILE */ > >@@ -790,7 +791,8 @@ static size_t rtnl_port_size(const struct net_device *dev) > > size_t port_self_size = nla_total_size(sizeof(struct nlattr)) > > + port_size; > > > >- if (!dev->netdev_ops->ndo_get_vf_port || !dev->dev.parent) > >+ if (!dev->netdev_ops->ndo_get_vf_port || !dev->dev.parent > >+ || !(ext_filter_mask & RTEXT_FILTER_VF)) > > Do not start line with || Ah, sorry, I'm getting my project codingstyles mixed up. Will fix. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [0/2] Fix problems with with IFLA_VF_PORTS 2014-04-23 7:21 [0/2] Fix problems with with IFLA_VF_PORTS David Gibson 2014-04-23 7:21 ` [PATCH 1/2] rtnetlink: Warn when interface's information won't fit in our packet David Gibson 2014-04-23 7:21 ` [PATCH 2/2] rtnetlink: Only supply IFLA_VF_PORTS information when RTEXT_FILTER_VF is set David Gibson @ 2014-04-23 15:14 ` Greg Rose 2 siblings, 0 replies; 12+ messages in thread From: Greg Rose @ 2014-04-23 15:14 UTC (permalink / raw) To: David Gibson Cc: netdev, ssujith, neepatel, benve, davem, ben, govindarajulu90 On Wed, 23 Apr 2014 17:21:03 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > I've had a customer encounter a problem with getifaddrs(3) freezing up > on a system with a Cisco enic device. > > I've discovered that the problem is caused by an enic device with a > large number of SR-IOV virtual functions overflowing the normal sized > packet buffer for netlink, leading to interfaces not being reported > from an RTM_GETLINK request. > > The first patch here just makes the problem easier to locate if it > occurs again in a different way, by adding a WARN_ON() when we run out > of room in a netlink packet in this manner. > > The second patch actually fixes the problem, by only reporting > IFLA_VF_PORTS information when the RTEXT_FILTER_VF flag is specified. > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Once you fix a couple of coding style errors mentioned by Jiri then Acked-by: Greg Rose <gregory.v.rose@intel.com> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [0/2] Fix problems with with IFLA_VF_PORTS (v2) @ 2014-04-24 0:22 David Gibson 2014-04-24 0:22 ` [PATCH 1/2] rtnetlink: Warn when interface's information won't fit in our packet David Gibson 0 siblings, 1 reply; 12+ messages in thread From: David Gibson @ 2014-04-24 0:22 UTC (permalink / raw) To: netdev Cc: ssujith, neepatel, benve, davem, ben, govindarajulu90, gregory.v.rose I've had a customer encounter a problem with getifaddrs(3) freezing up on a system with a Cisco enic device. I've discovered that the problem is caused by an enic device with a large number of SR-IOV virtual functions overflowing the normal sized packet buffer for netlink, leading to interfaces not being reported from an RTM_GETLINK request. The first patch here just makes the problem easier to locate if it occurs again in a different way, by adding a WARN_ON() when we run out of room in a netlink packet in this manner. The second patch actually fixes the problem, by only reporting IFLA_VF_PORTS information when the RTEXT_FILTER_VF flag is specified. v2: Corrected some CodingStyle problems ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/2] rtnetlink: Warn when interface's information won't fit in our packet 2014-04-24 0:22 [0/2] Fix problems with with IFLA_VF_PORTS (v2) David Gibson @ 2014-04-24 0:22 ` David Gibson 2014-04-24 15:49 ` Jiri Pirko 0 siblings, 1 reply; 12+ messages in thread From: David Gibson @ 2014-04-24 0:22 UTC (permalink / raw) To: netdev Cc: ssujith, neepatel, benve, davem, ben, govindarajulu90, gregory.v.rose, David Gibson Without IFLA_EXT_MASK specified, the information reported for a single interface in response to RTM_GETLINK is expected to fit within a netlink packet of NLMSG_GOODSIZE. If it doesn't, however, things will go badly wrong, When listing all interfaces, netlink_dump() will incorrectly treat -EMSGSIZE on the first message in a packet as the end of the listing and omit information for that interface and all subsequent ones. This can cause getifaddrs(3) to enter an infinite loop. This patch won't fix the problem, but it will WARN_ON() making it easier to track down what's going wrong. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- net/core/rtnetlink.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index d4ff417..b5e384a 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -1198,6 +1198,7 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) struct hlist_head *head; struct nlattr *tb[IFLA_MAX+1]; u32 ext_filter_mask = 0; + int err; s_h = cb->args[0]; s_idx = cb->args[1]; @@ -1218,11 +1219,17 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) hlist_for_each_entry_rcu(dev, head, index_hlist) { if (idx < s_idx) goto cont; - if (rtnl_fill_ifinfo(skb, dev, RTM_NEWLINK, - NETLINK_CB(cb->skb).portid, - cb->nlh->nlmsg_seq, 0, - NLM_F_MULTI, - ext_filter_mask) <= 0) + err = rtnl_fill_ifinfo(skb, dev, RTM_NEWLINK, + NETLINK_CB(cb->skb).portid, + cb->nlh->nlmsg_seq, 0, + NLM_F_MULTI, + ext_filter_mask); + /* If we ran out of room on the first message, + * we're in trouble + */ + WARN_ON((err == -EMSGSIZE) && (skb->len == 0)); + + if (err <= 0) goto out; nl_dump_check_consistent(cb, nlmsg_hdr(skb)); -- 1.9.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] rtnetlink: Warn when interface's information won't fit in our packet 2014-04-24 0:22 ` [PATCH 1/2] rtnetlink: Warn when interface's information won't fit in our packet David Gibson @ 2014-04-24 15:49 ` Jiri Pirko 0 siblings, 0 replies; 12+ messages in thread From: Jiri Pirko @ 2014-04-24 15:49 UTC (permalink / raw) To: David Gibson Cc: netdev, ssujith, neepatel, benve, davem, ben, govindarajulu90, gregory.v.rose Thu, Apr 24, 2014 at 02:22:35AM CEST, david@gibson.dropbear.id.au wrote: >Without IFLA_EXT_MASK specified, the information reported for a single >interface in response to RTM_GETLINK is expected to fit within a netlink >packet of NLMSG_GOODSIZE. > >If it doesn't, however, things will go badly wrong, When listing all >interfaces, netlink_dump() will incorrectly treat -EMSGSIZE on the first >message in a packet as the end of the listing and omit information for >that interface and all subsequent ones. This can cause getifaddrs(3) to >enter an infinite loop. > >This patch won't fix the problem, but it will WARN_ON() making it easier to >track down what's going wrong. > >Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Reviewed-by: Jiri Pirko <jpirko@redhat.com> ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2014-04-24 15:49 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-04-23 7:21 [0/2] Fix problems with with IFLA_VF_PORTS David Gibson 2014-04-23 7:21 ` [PATCH 1/2] rtnetlink: Warn when interface's information won't fit in our packet David Gibson 2014-04-23 9:09 ` Jiri Pirko 2014-04-23 11:41 ` David Gibson 2014-04-23 11:51 ` Jiri Pirko 2014-04-23 14:20 ` Sergei Shtylyov 2014-04-23 7:21 ` [PATCH 2/2] rtnetlink: Only supply IFLA_VF_PORTS information when RTEXT_FILTER_VF is set David Gibson 2014-04-23 9:17 ` Jiri Pirko 2014-04-24 0:13 ` David Gibson 2014-04-23 15:14 ` [0/2] Fix problems with with IFLA_VF_PORTS Greg Rose 2014-04-24 0:22 [0/2] Fix problems with with IFLA_VF_PORTS (v2) David Gibson 2014-04-24 0:22 ` [PATCH 1/2] rtnetlink: Warn when interface's information won't fit in our packet David Gibson 2014-04-24 15:49 ` Jiri Pirko
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.