| Message ID | 20181008200729.15392-2-mfo@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,Bionic/Cosmic/Unstable,1/1] rtnetlink: fix rtnl_fdb_dump() for ndmsg header | expand |
On 08.10.2018 22:07, Mauricio Faria de Oliveira wrote: > BugLink: https://bugs.launchpad.net/bugs/1796748 > > Currently, rtnl_fdb_dump() assumes the family header is 'struct ifinfomsg', > which is not always true -- 'struct ndmsg' is used by iproute2 ('ip neigh'). > > The problem is, the function bails out early if nlmsg_parse() fails, which > does occur for iproute2 usage of 'struct ndmsg' because the payload length > is shorter than the family header alone (as 'struct ifinfomsg' is assumed). > > This breaks backward compatibility with userspace -- nothing is sent back. > > Some examples with iproute2 and netlink library for go [1]: > > 1) $ bridge fdb show > 33:33:00:00:00:01 dev ens3 self permanent > 01:00:5e:00:00:01 dev ens3 self permanent > 33:33:ff:15:98:30 dev ens3 self permanent > > This one works, as it uses 'struct ifinfomsg'. > > fdb_show() @ iproute2/bridge/fdb.c > """ > .n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)), > ... > if (rtnl_dump_request(&rth, RTM_GETNEIGH, [...] > """ > > 2) $ ip --family bridge neigh > RTNETLINK answers: Invalid argument > Dump terminated > > This one fails, as it uses 'struct ndmsg'. > > do_show_or_flush() @ iproute2/ip/ipneigh.c > """ > .n.nlmsg_type = RTM_GETNEIGH, > .n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ndmsg)), > """ > > 3) $ ./neighlist > < no output > > > This one fails, as it uses 'struct ndmsg'-based. > > neighList() @ netlink/neigh_linux.go > """ > req := h.newNetlinkRequest(unix.RTM_GETNEIGH, [...] > msg := Ndmsg{ > """ > > The actual breakage was introduced by commit 0ff50e83b512 ("net: rtnetlink: > bail out from rtnl_fdb_dump() on parse error"), because nlmsg_parse() fails > if the payload length (with the _actual_ family header) is less than the > family header length alone (which is assumed, in parameter 'hdrlen'). > This is true in the examples above with struct ndmsg, with size and payload > length shorter than struct ifinfomsg. > > However, that commit just intends to fix something under the assumption the > family header is indeed an 'struct ifinfomsg' - by preventing access to the > payload as such (via 'ifm' pointer) if the payload length is not sufficient > to actually contain it. > > The assumption was introduced by commit 5e6d24358799 ("bridge: netlink dump > interface at par with brctl"), to support iproute2's 'bridge fdb' command > (not 'ip neigh') which indeed uses 'struct ifinfomsg', thus is not broken. > > So, in order to unbreak the 'struct ndmsg' family headers and still allow > 'struct ifinfomsg' to continue to work, check for the known message sizes > used with 'struct ndmsg' in iproute2 (with zero or one attribute which is > not used in this function anyway) then do not parse the data as ifinfomsg. > > Same examples with this patch applied (or revert/before the original fix): > > $ bridge fdb show > 33:33:00:00:00:01 dev ens3 self permanent > 01:00:5e:00:00:01 dev ens3 self permanent > 33:33:ff:15:98:30 dev ens3 self permanent > > $ ip --family bridge neigh > dev ens3 lladdr 33:33:00:00:00:01 PERMANENT > dev ens3 lladdr 01:00:5e:00:00:01 PERMANENT > dev ens3 lladdr 33:33:ff:15:98:30 PERMANENT > > $ ./neighlist > netlink.Neigh{LinkIndex:2, Family:7, State:128, Type:0, Flags:2, IP:net.IP(nil), HardwareAddr:net.HardwareAddr{0x33, 0x33, 0x0, 0x0, 0x0, 0x1}, LLIPAddr:net.IP(nil), Vlan:0, VNI:0} > netlink.Neigh{LinkIndex:2, Family:7, State:128, Type:0, Flags:2, IP:net.IP(nil), HardwareAddr:net.HardwareAddr{0x1, 0x0, 0x5e, 0x0, 0x0, 0x1}, LLIPAddr:net.IP(nil), Vlan:0, VNI:0} > netlink.Neigh{LinkIndex:2, Family:7, State:128, Type:0, Flags:2, IP:net.IP(nil), HardwareAddr:net.HardwareAddr{0x33, 0x33, 0xff, 0x15, 0x98, 0x30}, LLIPAddr:net.IP(nil), Vlan:0, VNI:0} > > Tested on mainline (v4.19-rc6) and net-next (3bd09b05b068). > > References: > > [1] netlink library for go (test-case) > https://github.com/vishvananda/netlink > > $ cat ~/go/src/neighlist/main.go > package main > import ("fmt"; "syscall"; "github.com/vishvananda/netlink") > func main() { > neighs, _ := netlink.NeighList(0, syscall.AF_BRIDGE) > for _, neigh := range neighs { fmt.Printf("%#v\n", neigh) } > } > > $ export GOPATH=~/go > $ go get github.com/vishvananda/netlink > $ go build neighlist > $ ~/go/src/neighlist/neighlist > > Thanks to David Ahern for suggestions to improve this patch. > > Fixes: 0ff50e83b512 ("net: rtnetlink: bail out from rtnl_fdb_dump() on parse error") > Fixes: 5e6d24358799 ("bridge: netlink dump interface at par with brctl") > Reported-by: Aidan Obley <aobley@pivotal.io> > Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com> > Reviewed-by: David Ahern <dsahern@gmail.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > (cherry picked from commit bd961c9bc66497f0c63f4ba1d02900bb85078366) > Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- > net/core/rtnetlink.c | 29 ++++++++++++++++++++--------- > 1 file changed, 20 insertions(+), 9 deletions(-) > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index fececbf6840a..45e21c8b698b 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -3554,16 +3554,27 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb) > int err = 0; > int fidx = 0; > > - err = nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb, > - IFLA_MAX, ifla_policy, NULL); > - if (err < 0) { > - return -EINVAL; > - } else if (err == 0) { > - if (tb[IFLA_MASTER]) > - br_idx = nla_get_u32(tb[IFLA_MASTER]); > - } > + /* A hack to preserve kernel<->userspace interface. > + * Before Linux v4.12 this code accepted ndmsg since iproute2 v3.3.0. > + * However, ndmsg is shorter than ifinfomsg thus nlmsg_parse() bails. > + * So, check for ndmsg with an optional u32 attribute (not used here). > + * Fortunately these sizes don't conflict with the size of ifinfomsg > + * with an optional attribute. > + */ > + if (nlmsg_len(cb->nlh) != sizeof(struct ndmsg) && > + (nlmsg_len(cb->nlh) != sizeof(struct ndmsg) + > + nla_attr_size(sizeof(u32)))) { > + err = nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb, > + IFLA_MAX, ifla_policy, NULL); > + if (err < 0) { > + return -EINVAL; > + } else if (err == 0) { > + if (tb[IFLA_MASTER]) > + br_idx = nla_get_u32(tb[IFLA_MASTER]); > + } > > - brport_idx = ifm->ifi_index; > + brport_idx = ifm->ifi_index; > + } > > if (br_idx) { > br_dev = __dev_get_by_index(net, br_idx); >
Upstream fix, clean cherry-pick, very well tested.
Acked-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Applied to cosmic master-next branch. Thanks. Cascardo.
Hi,
On Mon, Oct 8, 2018 at 5:07 PM Mauricio Faria de Oliveira
<mfo@canonical.com> wrote:
> BugLink: https://bugs.launchpad.net/bugs/1796748
Just checking whether this will be applied on Bionic in this SRU cycle as well?
Thanks!
On 10/15/18 16:53, Mauricio Faria de Oliveira wrote: > Hi, > > On Mon, Oct 8, 2018 at 5:07 PM Mauricio Faria de Oliveira > <mfo@canonical.com> wrote: >> BugLink: https://bugs.launchpad.net/bugs/1796748 > > Just checking whether this will be applied on Bionic in this SRU cycle as well? > > Thanks! > Hi Mauricio, The patch was reviewed and already has 2 ACK's so it's ready to be applied for the next SRU cycle. This is the 3rd week of the current cycle, so we'll start applying patches soon for the next which begins next week. Thanks, Kleber
Hi Kleber, Thanks for confirming! I guess I'm interpreting the dates in 'A new SRU cycle started' announces incorrectly... > 17-Oct Last day for kernel commits for this cycle. > 22-Oct - 27-Oct Kernel prep week. The 'last day for commits' is for patches/commits to be sent to the mailing list or to be applied to git trees? I guess my confusion is whether to interpret what is sent as _patches_, and what is applied in git as _commits_. Kind regards, Thanks! On Tue, Oct 16, 2018 at 4:46 AM Kleber Souza <kleber.souza@canonical.com> wrote: > > On 10/15/18 16:53, Mauricio Faria de Oliveira wrote: > > Hi, > > > > On Mon, Oct 8, 2018 at 5:07 PM Mauricio Faria de Oliveira > > <mfo@canonical.com> wrote: > >> BugLink: https://bugs.launchpad.net/bugs/1796748 > > > > Just checking whether this will be applied on Bionic in this SRU cycle as well? > > > > Thanks! > > > > Hi Mauricio, > > The patch was reviewed and already has 2 ACK's so it's ready to be > applied for the next SRU cycle. This is the 3rd week of the current > cycle, so we'll start applying patches soon for the next which begins > next week. > > > > Thanks, > Kleber
On 10/16/18 14:49, Mauricio Faria de Oliveira wrote: > Hi Kleber, > > Thanks for confirming! I guess I'm interpreting the dates in 'A new > SRU cycle started' announces incorrectly... > >> 17-Oct Last day for kernel commits for this cycle. >> 22-Oct - 27-Oct Kernel prep week. > > The 'last day for commits' is for patches/commits to be sent to the > mailing list or to be applied to git trees? > I guess my confusion is whether to interpret what is sent as > _patches_, and what is applied in git as _commits_. The "last day" is basically the deadline for having patches at least reviewed and ACK'ed on the mailing-list. Depending on the load of the team we might start applying the patches only later in the week, but if they are ACK'ed they are guaranteed to be picked up. Having that in mind, we don't guarantee though that if a patch series sent before the deadline will be reviewed and applied in time. An example would be patches that introduce a huge amount of changes or touch a sensitive part of the kernel, which could have a high potential for regressions. If we evaluate that more care is needed on the review of those patches, we might decide to postpone them to the next cycle. So the recommendation is to send the patches as soon as possible and nag someone if they didn't get enough attention when the deadline is getting close :-). Thanks, Kleber > > Kind regards, > Thanks! > > On Tue, Oct 16, 2018 at 4:46 AM Kleber Souza <kleber.souza@canonical.com> wrote: >> >> On 10/15/18 16:53, Mauricio Faria de Oliveira wrote: >>> Hi, >>> >>> On Mon, Oct 8, 2018 at 5:07 PM Mauricio Faria de Oliveira >>> <mfo@canonical.com> wrote: >>>> BugLink: https://bugs.launchpad.net/bugs/1796748 >>> >>> Just checking whether this will be applied on Bionic in this SRU cycle as well? >>> >>> Thanks! >>> >> >> Hi Mauricio, >> >> The patch was reviewed and already has 2 ACK's so it's ready to be >> applied for the next SRU cycle. This is the 3rd week of the current >> cycle, so we'll start applying patches soon for the next which begins >> next week. >> >> >> >> Thanks, >> Kleber > > >
On Tue, Oct 16, 2018 at 10:05 AM Kleber Souza <kleber.souza@canonical.com> wrote: > > On 10/16/18 14:49, Mauricio Faria de Oliveira wrote: > > Hi Kleber, > > > > Thanks for confirming! I guess I'm interpreting the dates in 'A new > > SRU cycle started' announces incorrectly... > > > >> 17-Oct Last day for kernel commits for this cycle. > >> 22-Oct - 27-Oct Kernel prep week. > > > > The 'last day for commits' is for patches/commits to be sent to the > > mailing list or to be applied to git trees? > > I guess my confusion is whether to interpret what is sent as > > _patches_, and what is applied in git as _commits_. > > The "last day" is basically the deadline for having patches at least > reviewed and ACK'ed on the mailing-list. Depending on the load of the > team we might start applying the patches only later in the week, but if > they are ACK'ed they are guaranteed to be picked up. Ah, now I got it. Last day for patches to get enough ACKs. :- ) Noted! > Having that in mind, we don't guarantee though that if a patch series > sent before the deadline will be reviewed and applied in time. An > example would be patches that introduce a huge amount of changes or > touch a sensitive part of the kernel, which could have a high potential > for regressions. If we evaluate that more care is needed on the review > of those patches, we might decide to postpone them to the next cycle. Sure, certainly understandable. > So the recommendation is to send the patches as soon as possible and nag > someone if they didn't get enough attention when the deadline is getting > close :-). Ok, right. Thank you very much for the detailed explanation. > > > Thanks, > Kleber > > > > > Kind regards, > > Thanks! > > > > On Tue, Oct 16, 2018 at 4:46 AM Kleber Souza <kleber.souza@canonical.com> wrote: > >> > >> On 10/15/18 16:53, Mauricio Faria de Oliveira wrote: > >>> Hi, > >>> > >>> On Mon, Oct 8, 2018 at 5:07 PM Mauricio Faria de Oliveira > >>> <mfo@canonical.com> wrote: > >>>> BugLink: https://bugs.launchpad.net/bugs/1796748 > >>> > >>> Just checking whether this will be applied on Bionic in this SRU cycle as well? > >>> > >>> Thanks! > >>> > >> > >> Hi Mauricio, > >> > >> The patch was reviewed and already has 2 ACK's so it's ready to be > >> applied for the next SRU cycle. This is the 3rd week of the current > >> cycle, so we'll start applying patches soon for the next which begins > >> next week. > >> > >> > >> > >> Thanks, > >> Kleber > > > > > > >
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index fececbf6840a..45e21c8b698b 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -3554,16 +3554,27 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb) int err = 0; int fidx = 0; - err = nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb, - IFLA_MAX, ifla_policy, NULL); - if (err < 0) { - return -EINVAL; - } else if (err == 0) { - if (tb[IFLA_MASTER]) - br_idx = nla_get_u32(tb[IFLA_MASTER]); - } + /* A hack to preserve kernel<->userspace interface. + * Before Linux v4.12 this code accepted ndmsg since iproute2 v3.3.0. + * However, ndmsg is shorter than ifinfomsg thus nlmsg_parse() bails. + * So, check for ndmsg with an optional u32 attribute (not used here). + * Fortunately these sizes don't conflict with the size of ifinfomsg + * with an optional attribute. + */ + if (nlmsg_len(cb->nlh) != sizeof(struct ndmsg) && + (nlmsg_len(cb->nlh) != sizeof(struct ndmsg) + + nla_attr_size(sizeof(u32)))) { + err = nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb, + IFLA_MAX, ifla_policy, NULL); + if (err < 0) { + return -EINVAL; + } else if (err == 0) { + if (tb[IFLA_MASTER]) + br_idx = nla_get_u32(tb[IFLA_MASTER]); + } - brport_idx = ifm->ifi_index; + brport_idx = ifm->ifi_index; + } if (br_idx) { br_dev = __dev_get_by_index(net, br_idx);
BugLink: https://bugs.launchpad.net/bugs/1796748 Currently, rtnl_fdb_dump() assumes the family header is 'struct ifinfomsg', which is not always true -- 'struct ndmsg' is used by iproute2 ('ip neigh'). The problem is, the function bails out early if nlmsg_parse() fails, which does occur for iproute2 usage of 'struct ndmsg' because the payload length is shorter than the family header alone (as 'struct ifinfomsg' is assumed). This breaks backward compatibility with userspace -- nothing is sent back. Some examples with iproute2 and netlink library for go [1]: 1) $ bridge fdb show 33:33:00:00:00:01 dev ens3 self permanent 01:00:5e:00:00:01 dev ens3 self permanent 33:33:ff:15:98:30 dev ens3 self permanent This one works, as it uses 'struct ifinfomsg'. fdb_show() @ iproute2/bridge/fdb.c """ .n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)), ... if (rtnl_dump_request(&rth, RTM_GETNEIGH, [...] """ 2) $ ip --family bridge neigh RTNETLINK answers: Invalid argument Dump terminated This one fails, as it uses 'struct ndmsg'. do_show_or_flush() @ iproute2/ip/ipneigh.c """ .n.nlmsg_type = RTM_GETNEIGH, .n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ndmsg)), """ 3) $ ./neighlist < no output > This one fails, as it uses 'struct ndmsg'-based. neighList() @ netlink/neigh_linux.go """ req := h.newNetlinkRequest(unix.RTM_GETNEIGH, [...] msg := Ndmsg{ """ The actual breakage was introduced by commit 0ff50e83b512 ("net: rtnetlink: bail out from rtnl_fdb_dump() on parse error"), because nlmsg_parse() fails if the payload length (with the _actual_ family header) is less than the family header length alone (which is assumed, in parameter 'hdrlen'). This is true in the examples above with struct ndmsg, with size and payload length shorter than struct ifinfomsg. However, that commit just intends to fix something under the assumption the family header is indeed an 'struct ifinfomsg' - by preventing access to the payload as such (via 'ifm' pointer) if the payload length is not sufficient to actually contain it. The assumption was introduced by commit 5e6d24358799 ("bridge: netlink dump interface at par with brctl"), to support iproute2's 'bridge fdb' command (not 'ip neigh') which indeed uses 'struct ifinfomsg', thus is not broken. So, in order to unbreak the 'struct ndmsg' family headers and still allow 'struct ifinfomsg' to continue to work, check for the known message sizes used with 'struct ndmsg' in iproute2 (with zero or one attribute which is not used in this function anyway) then do not parse the data as ifinfomsg. Same examples with this patch applied (or revert/before the original fix): $ bridge fdb show 33:33:00:00:00:01 dev ens3 self permanent 01:00:5e:00:00:01 dev ens3 self permanent 33:33:ff:15:98:30 dev ens3 self permanent $ ip --family bridge neigh dev ens3 lladdr 33:33:00:00:00:01 PERMANENT dev ens3 lladdr 01:00:5e:00:00:01 PERMANENT dev ens3 lladdr 33:33:ff:15:98:30 PERMANENT $ ./neighlist netlink.Neigh{LinkIndex:2, Family:7, State:128, Type:0, Flags:2, IP:net.IP(nil), HardwareAddr:net.HardwareAddr{0x33, 0x33, 0x0, 0x0, 0x0, 0x1}, LLIPAddr:net.IP(nil), Vlan:0, VNI:0} netlink.Neigh{LinkIndex:2, Family:7, State:128, Type:0, Flags:2, IP:net.IP(nil), HardwareAddr:net.HardwareAddr{0x1, 0x0, 0x5e, 0x0, 0x0, 0x1}, LLIPAddr:net.IP(nil), Vlan:0, VNI:0} netlink.Neigh{LinkIndex:2, Family:7, State:128, Type:0, Flags:2, IP:net.IP(nil), HardwareAddr:net.HardwareAddr{0x33, 0x33, 0xff, 0x15, 0x98, 0x30}, LLIPAddr:net.IP(nil), Vlan:0, VNI:0} Tested on mainline (v4.19-rc6) and net-next (3bd09b05b068). References: [1] netlink library for go (test-case) https://github.com/vishvananda/netlink $ cat ~/go/src/neighlist/main.go package main import ("fmt"; "syscall"; "github.com/vishvananda/netlink") func main() { neighs, _ := netlink.NeighList(0, syscall.AF_BRIDGE) for _, neigh := range neighs { fmt.Printf("%#v\n", neigh) } } $ export GOPATH=~/go $ go get github.com/vishvananda/netlink $ go build neighlist $ ~/go/src/neighlist/neighlist Thanks to David Ahern for suggestions to improve this patch. Fixes: 0ff50e83b512 ("net: rtnetlink: bail out from rtnl_fdb_dump() on parse error") Fixes: 5e6d24358799 ("bridge: netlink dump interface at par with brctl") Reported-by: Aidan Obley <aobley@pivotal.io> Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com> Reviewed-by: David Ahern <dsahern@gmail.com> Signed-off-by: David S. Miller <davem@davemloft.net> (cherry picked from commit bd961c9bc66497f0c63f4ba1d02900bb85078366) Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com> --- net/core/rtnetlink.c | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-)