netdev - RE: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

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]

Message-ID: <77EF4405DD4BB54AACCE7DB593DF6A9A9E303A@SJEXCHMB14.corp.ad.broadcom.com>
Date:	Thu, 20 Aug 2015 04:56:16 +0000
From:	Premkumar Jonnala <pjonnala@...adcom.com>
To:	Scott Feldman <sfeldma@...il.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH] bridge: Enable configuration of ageing interval for
 bridges and switch devices.

Thank you Scott.  Please see inline.

> >> -----Original Message-----
> >> From: Scott Feldman [mailto:sfeldma@...il.com]
> >> Sent: Tuesday, August 18, 2015 12:48 PM
> >> To: Premkumar Jonnala
> >> Cc: netdev@...r.kernel.org
> >> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for
> bridges
> >> and switch devices.
> >>
> >>
> >>
> >> On Fri, 14 Aug 2015, Premkumar Jonnala wrote:
> >>
> >> > Bridge devices have ageing interval used to age out MAC addresses
> >> > from FDB.  This ageing interval was not configuratble.
> >> >
> >> > Enable netlink based configuration of ageing interval for bridges and
> >> > switch devices.  The ageing interval changes the timer used to purge
> >> > inactive FDB entries in bridges.  The ageing interval config is
> >> > propagated to switch devices, so that platform or hardware based
> >> > ageing works according to configuration.
> >> >
> >> > Signed-off-by: Premkumar Jonnala <pjonnala@...adcom.com>
> >>
> >> Hi Premkumar,
> >>
> >> I agree with Roopa that we should use existing IFLA_BR_AGEING_TIME.
> >
> > What is the motivation for using 'ip link' command to configure bridge
> attributes?  IMHO,
> > bridge command is better suited for that.
> 
> Can you extend bridge command to allow setting/getting these bridge
> attrs?  Looks like you construct a RTM_NEWLINK IFLA_INFO_DATA msg.  No
> changes needed to the kernel.
> 
> bridge link set dev br0 ageing_time 1000
> 
>  --or--
> 
> ip link set dev br0 type bridge ageing_time 1000

I'd prefer to deprecate/remove all the 6 options on the 'ip link' command and move them to 'bridge' command.  

> 
> >> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> >> index 0f2408f..01401ea 100644
> >> --- a/net/bridge/br_netlink.c
> >> +++ b/net/bridge/br_netlink.c
> >> @@ -759,9 +759,9 @@ static int br_changelink(struct net_device *brdev,
> struct
> >> nlattr *tb[],
> >>       }
> >>
> >>       if (data[IFLA_BR_AGEING_TIME]) {
> >> -             u32 ageing_time = nla_get_u32(data[IFLA_BR_AGEING_TIME]);
> >
> > Should we do some range checking here to ensure that the value is within a
> certain range.
> > IEEE 802.1d recommends that the ageing time be between 10 sec and 1 million
> seconds.
> 
> Sure, but make that a separate patch.

Sure.

> 
> >> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
> >> +{
> >> +     struct switchdev_attr attr = {
> >> +             .id = SWITCHDEV_ATTR_BRIDGE,
> >> +             .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
> >> +             .u.bridge.attr = IFLA_BR_AGEING_TIME,
> >> +             .u.bridge.val = ageing_time,
> >> +     };
> >> +     int err;
> >> +
> >> +     err = switchdev_port_attr_set(br->dev, &attr);
> >> +     if (err)
> >> +             return err;
> >> +
> >> +     br->ageing_time = clock_t_to_jiffies(ageing_time);
> >
> > Should we restart the timer here the new time takes effect?
> 
> I don't know...I just copied what the original code did.  If it does
> need to be restarted, break that out as a separate patch.

There is another comment on this thread where it was suggested that we restart the timer.  I will
make the change in a separate patch.

-Prem

> 
> -scott