netdev - Re: [PATCH net-next 05/15] devlink: Allow setting of packet trap group parameters

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: <20200325094524.5c84a510@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 25 Mar 2020 09:45:24 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, jiri@...lanox.com,
        andrew@...n.ch, f.fainelli@...il.com, vivien.didelot@...il.com,
        roopa@...ulusnetworks.com, nikolay@...ulusnetworks.com,
        mlxsw@...lanox.com, Ido Schimmel <idosch@...lanox.com>
Subject: Re: [PATCH net-next 05/15] devlink: Allow setting of packet trap
 group parameters

On Wed, 25 Mar 2020 12:37:43 +0200 Ido Schimmel wrote:
> > > +		if (policer_id && !policer_item) {
> > > +			NL_SET_ERR_MSG_MOD(extack, "Device did not register this trap policer");  
> > 
> > nit: is KBUILD_MODNAME still set if devlink can only be built-in now?  
> 
> It seems fine:
> 
> NL_SET_ERR_MSG_MOD:
> 
> # devlink trap policer set pci/0000:01:00.0 policer 1337
> Error: devlink: Device did not register this trap policer.
> devlink answers: No such file or directory
> 
> NL_SET_ERR_MSG:
> 
> # devlink trap policer set pci/0000:01:00.0 policer 1337
> Error: Device did not register this trap policer.
> devlink answers: No such file or directory

GTK!

> > > +			return -ENOENT;
> > > +		}
> > > +	}
> > > +	policer = policer_item ? policer_item->policer : NULL;
> > > +
> > > +	err = devlink->ops->trap_group_set(devlink, group_item->group, policer);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	group_item->policer_item = policer_item;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int devlink_nl_cmd_trap_group_set_doit(struct sk_buff *skb,
> > >  					      struct genl_info *info)
> > >  {
> > > @@ -6060,6 +6099,10 @@ static int devlink_nl_cmd_trap_group_set_doit(struct sk_buff *skb,
> > >  	if (err)
> > >  		return err;
> > >  
> > > +	err = devlink_trap_group_set(devlink, group_item, info);
> > > +	if (err)
> > > +		return err;  
> > 
> > Should this unwind the action changes? Are the changes supposed to be
> > atomic? :S   
> 
> I used do_setlink() as reference and it seems that it does not unwind
> the changes. I can add extack message in case we did set action and
> devlink_trap_group_set() failed.

Okay.

> > Also could it potentially be a problem if trap is being enabled and
> > policer applied - if we enable first the CPU may get overloaded and it
> > may be hard to apply the policer? Making sure the ordering is right
> > requires some careful checking, so IDK if its worth it..  
> 
> I'm not sure it's really an issue, but I can flip the order just to be
> on the safe side.

No, no, flipping doesn't do anything. It will just move the problem
from enable to disable. You can leave as is if it's not expected to 
be an issue.

> >   
> > >  	return 0;
> > >  }
> > >    
> >