* [PATCH iproute2-next] bridge: fdb: Fix filtering with strict checking enabled / disabled
@ 2019-01-01 7:18 Ido Schimmel
2019-01-03 4:34 ` David Ahern
0 siblings, 1 reply; 3+ messages in thread
From: Ido Schimmel @ 2019-01-01 7:18 UTC (permalink / raw)
To: netdev@vger.kernel.org
Cc: dsahern@gmail.com, stephen@networkplumber.org, Ido Schimmel
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:
# bridge fdb show br br0
Error: Unsupported attribute in fdb dump request.
Dump terminated
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.
Fix this by adding a new flag to the RTNL handle which indicates whether
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.
Tested with and without strict checking on net-next and on older kernels
(v4.18, v4.17, v4.9).
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 <idosch@mellanox.com>
---
bridge/fdb.c | 11 ++++++++++-
include/libnetlink.h | 1 +
lib/libnetlink.c | 6 ++++--
3 files changed, 15 insertions(+), 3 deletions(-)
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 = NULL;
int msg_size = sizeof(struct ndmsg);
+ if (!(rth.flags & RTNL_HANDLE_F_STRICT_CHK)) {
+ req.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg));
+ msg_size = sizeof(struct ifinfomsg);
+ }
+
while (argc > 0) {
if ((strcmp(*argv, "brport") == 0) || strcmp(*argv, "dev") == 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 += RTA_LENGTH(4);
}
diff --git a/include/libnetlink.h b/include/libnetlink.h
index dc0c9c4eb3f5..5c3ef4025d81 100644
--- a/include/libnetlink.h
+++ b/include/libnetlink.h
@@ -23,6 +23,7 @@ struct rtnl_handle {
FILE *dump_fp;
#define RTNL_HANDLE_F_LISTEN_ALL_NSID 0x01
#define RTNL_HANDLE_F_SUPPRESS_NLERR 0x02
+#define RTNL_HANDLE_F_STRICT_CHK 0x04
int flags;
};
diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 4d7d081054fd..c43efaa8c55b 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -166,8 +166,10 @@ void rtnl_set_strict_dump(struct rtnl_handle *rth)
{
int one = 1;
- setsockopt(rth->fd, SOL_NETLINK, NETLINK_GET_STRICT_CHK,
- &one, sizeof(one));
+ if (setsockopt(rth->fd, SOL_NETLINK, NETLINK_GET_STRICT_CHK,
+ &one, sizeof(one)) < 0)
+ return;
+ rth->flags |= RTNL_HANDLE_F_STRICT_CHK;
}
void rtnl_close(struct rtnl_handle *rth)
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH iproute2-next] bridge: fdb: Fix filtering with strict checking enabled / disabled
2019-01-01 7:18 [PATCH iproute2-next] bridge: fdb: Fix filtering with strict checking enabled / disabled Ido Schimmel
@ 2019-01-03 4:34 ` David Ahern
2019-01-04 11:25 ` Ido Schimmel
0 siblings, 1 reply; 3+ messages in thread
From: David Ahern @ 2019-01-03 4:34 UTC (permalink / raw)
To: Ido Schimmel, netdev@vger.kernel.org; +Cc: stephen@networkplumber.org
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:
>
> # bridge fdb show br br0
> Error: Unsupported attribute in fdb dump request.
> Dump terminated
>
> 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.
>
> Fix this by adding a new flag to the RTNL handle which indicates whether
> 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.
>
> Tested with and without strict checking on net-next and on older kernels
> (v4.18, v4.17, v4.9).
>
> 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 <idosch@mellanox.com>
> ---
> bridge/fdb.c | 11 ++++++++++-
> include/libnetlink.h | 1 +
> lib/libnetlink.c | 6 ++++--
> 3 files changed, 15 insertions(+), 3 deletions(-)
>
> 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 = NULL;
> int msg_size = sizeof(struct ndmsg);
>
> + if (!(rth.flags & RTNL_HANDLE_F_STRICT_CHK)) {
> + req.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg));
> + msg_size = sizeof(struct ifinfomsg);
> + }
> +
> while (argc > 0) {
> if ((strcmp(*argv, "brport") == 0) || strcmp(*argv, "dev") == 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 += RTA_LENGTH(4);
> }
>
I like the addition of the flag to rth as a way for commands to know if
the kernel supports strict checking.
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:
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 =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();
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.
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,
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH iproute2-next] bridge: fdb: Fix filtering with strict checking enabled / disabled
2019-01-03 4:34 ` David Ahern
@ 2019-01-04 11:25 ` Ido Schimmel
0 siblings, 0 replies; 3+ messages in thread
From: Ido Schimmel @ 2019-01-04 11:25 UTC (permalink / raw)
To: David Ahern; +Cc: netdev@vger.kernel.org, stephen@networkplumber.org
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:
> >
> > # bridge fdb show br br0
> > Error: Unsupported attribute in fdb dump request.
> > Dump terminated
> >
> > 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.
> >
> > Fix this by adding a new flag to the RTNL handle which indicates whether
> > 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.
> >
> > Tested with and without strict checking on net-next and on older kernels
> > (v4.18, v4.17, v4.9).
> >
> > 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 <idosch@mellanox.com>
> > ---
> > bridge/fdb.c | 11 ++++++++++-
> > include/libnetlink.h | 1 +
> > lib/libnetlink.c | 6 ++++--
> > 3 files changed, 15 insertions(+), 3 deletions(-)
> >
> > 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 = NULL;
> > int msg_size = sizeof(struct ndmsg);
> >
> > + if (!(rth.flags & RTNL_HANDLE_F_STRICT_CHK)) {
> > + req.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg));
> > + msg_size = sizeof(struct ifinfomsg);
> > + }
> > +
> > while (argc > 0) {
> > if ((strcmp(*argv, "brport") == 0) || strcmp(*argv, "dev") == 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 += RTA_LENGTH(4);
> > }
> >
>
> I like the addition of the flag to rth as a way for commands to know if
> the kernel supports strict checking.
>
> 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.
>
> 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 =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();
>
>
> 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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-01-04 11:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-01-01 7:18 [PATCH iproute2-next] bridge: fdb: Fix filtering with strict checking enabled / disabled Ido Schimmel
2019-01-03 4:34 ` David Ahern
2019-01-04 11:25 ` Ido Schimmel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).