netdev - Re: [PATCH iproute2-next] bridge: Add backup nexthop ID support

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 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 2 Aug 2023 15:34:15 +0300
From: Ido Schimmel <idosch@...sch.org>
To: Petr Machata <petrm@...dia.com>
Cc: Ido Schimmel <idosch@...dia.com>, netdev@...r.kernel.org,
	dsahern@...il.com, stephen@...workplumber.org, razor@...ckwall.org
Subject: Re: [PATCH iproute2-next] bridge: Add backup nexthop ID support

On Wed, Aug 02, 2023 at 11:55:26AM +0200, Petr Machata wrote:
> 
> Ido Schimmel <idosch@...dia.com> writes:
> 
> > diff --git a/bridge/link.c b/bridge/link.c
> > index b35429866f52..c7ee5e760c08 100644
> > --- a/bridge/link.c
> > +++ b/bridge/link.c
> > @@ -186,6 +186,10 @@ static void print_protinfo(FILE *fp, struct rtattr *attr)
> >  				     ll_index_to_name(ifidx));
> >  		}
> >  
> > +		if (prtb[IFLA_BRPORT_BACKUP_NHID])
> > +			print_uint(PRINT_ANY, "backup_nhid", "backup_nhid %u ",
> > +				   rta_getattr_u32(prtb[IFLA_BRPORT_BACKUP_NHID]));
> > +
> 
> This doesn't build on current main. I think we usually send the relevant
> header sync patch, but maybe there's an assumption the maintainer pushes
> it _before_ this patch? I'm not sure, just calling it out.

Not needed. David syncs the headers himself.

> 
> >  		if (prtb[IFLA_BRPORT_ISOLATED])
> >  			print_on_off(PRINT_ANY, "isolated", "isolated %s ",
> >  				     rta_getattr_u8(prtb[IFLA_BRPORT_ISOLATED]));
> > @@ -311,6 +315,7 @@ static void usage(void)
> >  		"                               [ mab {on | off} ]\n"
> >  		"                               [ hwmode {vepa | veb} ]\n"
> >  		"                               [ backup_port DEVICE ] [ nobackup_port ]\n"
> > +		"                               [ backup_nhid NHID ]\n"
> 
> I thought about whether there should be "nobackup_nhid", but no. The
> corresponding nobackup_port is necessary because it would be awkward to
> specify "backup_port ''" or something. No such issue with NHID.
> 
> >  		"                               [ self ] [ master ]\n"
> >  		"       bridge link show [dev DEV]\n");
> >  	exit(-1);
> > @@ -330,6 +335,7 @@ static int brlink_modify(int argc, char **argv)
> >  	};
> >  	char *d = NULL;
> >  	int backup_port_idx = -1;
> > +	__s32 backup_nhid = -1;
> >  	__s8 neigh_suppress = -1;
> >  	__s8 neigh_vlan_suppress = -1;
> >  	__s8 learning = -1;
> > @@ -493,6 +499,10 @@ static int brlink_modify(int argc, char **argv)
> >  			}
> >  		} else if (strcmp(*argv, "nobackup_port") == 0) {
> >  			backup_port_idx = 0;
> > +		} else if (strcmp(*argv, "backup_nhid") == 0) {
> > +			NEXT_ARG();
> > +			if (get_s32(&backup_nhid, *argv, 0))
> > +				invarg("invalid backup_nhid", *argv);
> 
> Not sure about that s32. NHID's are unsigned in general. I can add a
> NHID of 0xffffffff just fine:
> 
> # ip nexthop add id 0xffffffff via 192.0.2.3 dev Xd
> 
> (Though ip nexthop show then loops endlessly probably because -1 is used
> as a sentinel in the dump code. Oops!)
> 
> IMHO the tool should allow configuring this. You allow full u32 range
> for the "ip" tool, no need for "bridge" to be arbitrarily limited.

What about the diff below?

diff --git a/bridge/link.c b/bridge/link.c
index c7ee5e760c08..4bf806c5be61 100644
--- a/bridge/link.c
+++ b/bridge/link.c
@@ -334,8 +334,9 @@ static int brlink_modify(int argc, char **argv)
                .ifm.ifi_family = PF_BRIDGE,
        };
        char *d = NULL;
+       bool backup_nhid_set = false;
+       __u32 backup_nhid;
        int backup_port_idx = -1;
-       __s32 backup_nhid = -1;
        __s8 neigh_suppress = -1;
        __s8 neigh_vlan_suppress = -1;
        __s8 learning = -1;
@@ -501,8 +502,9 @@ static int brlink_modify(int argc, char **argv)
                        backup_port_idx = 0;
                } else if (strcmp(*argv, "backup_nhid") == 0) {
                        NEXT_ARG();
-                       if (get_s32(&backup_nhid, *argv, 0))
+                       if (get_u32(&backup_nhid, *argv, 0))
                                invarg("invalid backup_nhid", *argv);
+                       backup_nhid_set = true;
                } else {
                        usage();
                }
@@ -589,7 +591,7 @@ static int brlink_modify(int argc, char **argv)
                addattr32(&req.n, sizeof(req), IFLA_BRPORT_BACKUP_PORT,
                          backup_port_idx);
 
-       if (backup_nhid != -1)
+       if (backup_nhid_set)
                addattr32(&req.n, sizeof(req), IFLA_BRPORT_BACKUP_NHID,
                          backup_nhid);