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: <77EF4405DD4BB54AACCE7DB593DF6A9A9E3076@SJEXCHMB14.corp.ad.broadcom.com>
Date:	Thu, 20 Aug 2015 05:08:51 +0000
From:	Premkumar Jonnala <pjonnala@...adcom.com>
To:	"Wilson, Daniel G" <daniel.wilson@...el.com>,
	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.

Hi Daniel,

Thank you for the comments.  Please see inline.

> -----Original Message-----
> From: Wilson, Daniel G [mailto:daniel.wilson@...el.com]
> Sent: Wednesday, August 19, 2015 11:33 PM
> To: Scott Feldman; Premkumar Jonnala
> Cc: netdev@...r.kernel.org
> Subject: RE: [PATCH] bridge: Enable configuration of ageing interval for bridges
> and switch devices.
> 
> > -----Original Message-----
> > From: netdev-owner@...r.kernel.org [mailto:netdev-
> owner@...r.kernel.org]
> > On Behalf Of Scott Feldman
> > Sent: Wednesday, August 19, 2015 12:54 PM
> > To: Premkumar Jonnala
> > Cc: netdev@...r.kernel.org
> > Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for
> bridges
> > and switch devices.
> >
> > On Wed, Aug 19, 2015 at 2:34 AM, Premkumar Jonnala
> > <pjonnala@...adcom.com> wrote:
> > > Hello Scott,
> > >
> > > Thank you for the diff and comments.   Please see my comments 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

IMHO, we should choose only one command.  Otherwise, we'd have to spend effort in trying to keep both the commands in sync.
My vote would be for the bridge command - since the options/parameters are related to bridges.  If there is no objection,
I'll move all the bridge options from 'ip link' command to 'bridge' command.

> 
> Being able to set these attributes via both bridge and ip would be great.
> 
> > >> +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.
> 
> In my opinion, yes, the timer should be restarted. If the timer had been set to 1
> million seconds and is being changed to 1 minute, you wouldn't want to wait for
> the 1-million-second  timer to expire before resetting it to the newly-configured
> 1-minute timer value.

I agree with you totally.  This is exactly the reason I want to restart the timer.  I'll
make the changes and post the patch.

-Prem

> 
> Dan.