From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ido Schimmel Subject: Re: [PATCH iproute2-next] bridge: fdb: Fix filtering with strict checking enabled / disabled Date: Fri, 4 Jan 2019 11:25:34 +0000 Message-ID: <20190104112532.GA3471@splinter> References: <20190101071828.8157-1-idosch@mellanox.com> <74ba42e9-2efe-9c4d-666b-9b224089c2bd@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "netdev@vger.kernel.org" , "stephen@networkplumber.org" To: David Ahern Return-path: Received: from mail-eopbgr130074.outbound.protection.outlook.com ([40.107.13.74]:16803 "EHLO EUR01-HE1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726404AbfADLZm (ORCPT ); Fri, 4 Jan 2019 06:25:42 -0500 In-Reply-To: <74ba42e9-2efe-9c4d-666b-9b224089c2bd@gmail.com> Content-Language: en-US Content-ID: <4FB5CBA7698E1748AF9DFACE6358C1FE@eurprd05.prod.outlook.com> Sender: netdev-owner@vger.kernel.org List-ID: On Wed, Jan 02, 2019 at 09:34:28PM -0700, David Ahern wrote: > On 1/1/19 12:18 AM, Ido Schimmel wrote: > > When strict checking is enabled the kernel expects to receive the > > ifindex of the bridge device using 'NDA_MASTER', but iproute2 currently > > uses 'IFLA_MASTER' which the kernel expects when strict checking is > > disabled. Therefore, using iproute2 on current kernels while filtering > > on bridge results in the following error: > >=20 > > # bridge fdb show br br0 > > Error: Unsupported attribute in fdb dump request. > > Dump terminated > >=20 > > Additionally, when strict checking is disabled and the bridge is > > specified via 'IFLA_MASTER', we need to make sure that the message > > length actually corresponds to 'struct ifinfomsg' and a potential > > attribute. > >=20 > > Fix this by adding a new flag to the RTNL handle which indicates whethe= r > > strict checking is enabled on the socket or not. If it is enabled, > > specify 'NDA_MASTER'. Otherwise, specify 'IFLA_MASTER' and set the > > message length accordingly. > >=20 > > Tested with and without strict checking on net-next and on older kernel= s > > (v4.18, v4.17, v4.9). > >=20 > > Fixes: 66e8e73edc65 ("bridge: fdb: Use 'struct ndmsg' for FDB dumping") > > Fixes: aea41afcfd6d ("ip bridge: Set NETLINK_GET_STRICT_CHK on socket") > > Signed-off-by: Ido Schimmel > > --- > > bridge/fdb.c | 11 ++++++++++- > > include/libnetlink.h | 1 + > > lib/libnetlink.c | 6 ++++-- > > 3 files changed, 15 insertions(+), 3 deletions(-) > >=20 > > diff --git a/bridge/fdb.c b/bridge/fdb.c > > index a7a0d8052307..f898b20918fb 100644 I > > --- a/bridge/fdb.c > > +++ b/bridge/fdb.c > > @@ -271,6 +271,11 @@ static int fdb_show(int argc, char **argv) > > char *br =3D NULL; > > int msg_size =3D sizeof(struct ndmsg); > > =20 > > + if (!(rth.flags & RTNL_HANDLE_F_STRICT_CHK)) { > > + req.n.nlmsg_len =3D NLMSG_LENGTH(sizeof(struct ifinfomsg)); > > + msg_size =3D sizeof(struct ifinfomsg); > > + } > > + > > while (argc > 0) { > > if ((strcmp(*argv, "brport") =3D=3D 0) || strcmp(*argv, "dev") =3D= =3D 0) { > > NEXT_ARG(); > > @@ -304,7 +309,11 @@ static int fdb_show(int argc, char **argv) > > fprintf(stderr, "Cannot find bridge device \"%s\"\n", br); > > return -1; > > } > > - addattr32(&req.n, sizeof(req), IFLA_MASTER, br_ifindex); > > + > > + if (rth.flags & RTNL_HANDLE_F_STRICT_CHK) > > + addattr32(&req.n, sizeof(req), NDA_MASTER, br_ifindex); > > + else > > + addattr32(&req.n, sizeof(req), IFLA_MASTER, br_ifindex); > > msg_size +=3D RTA_LENGTH(4); > > } > > =20 >=20 > I like the addition of the flag to rth as a way for commands to know if > the kernel supports strict checking. >=20 > Couple of things. first, the patch (at least when I saved it to file) > has html codes in it. New for a patch from you so not sure what > happened. An example: No clue. I didn't do anything differently. >=20 > diff --git a/bridge/fdb.c b/bridge/fdb.c > index a7a0d8052307..f898b20918fb 100644 > --- a/bridge/fdb.c > +++ b/bridge/fdb.c > @@ -271,6 +271,11 @@ static int fdb_show(int argc, char **argv) > char *br =3D3D NULL; > int msg_size =3D3D sizeof(struct ndmsg); > =3D20 > + if (!(rth.flags & RTNL_HANDLE_F_STRICT_CHK)) { > + req.n.nlmsg_len =3D3D NLMSG_LENGTH(sizeof(struct ifinfoms= g)); > + msg_size =3D3D sizeof(struct ifinfomsg); > + } > + > while (argc > 0) { > if ((strcmp(*argv, "brport") =3D3D=3D3D 0) || strcmp(*arg= v, > "dev") =3D3D=3D3D 0)=3D > { > NEXT_ARG(); >=20 >=20 > Second, the current code after your last patch sets the ifindex in the > ancillary header based on ndmsg versus ifinfomsg. While the offset is > the same, it seems odd and a challenge for future readers that the > master attribute toggles between IFLA and NDA but the struct entry does > not matter. Yes, I agree it's not pretty. > I think long term the code should be consistent with other dump > commands. I missed neigh and fdb dumps in my first round. Specifically, > removing the req from the list functions and instead using the type > specific dump functions with a filter function that can append to the > request when it makes sense. I have that coded and seems to work fine on > latest kernel and older (4.1). If you have some time another > verification would be great. Thanks, I'll take a look.