netdev - Re: [patch net-next v4 01/10] flow_offload: Introduce offload of HW stats type

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: <20200309143630.2f83476f@kicinski-fedora-PC1C0HJN>
Date:   Mon, 9 Mar 2020 14:36:30 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Edward Cree <ecree@...arflare.com>
Cc:     Jiri Pirko <jiri@...nulli.us>, <netdev@...r.kernel.org>,
        <davem@...emloft.net>, <saeedm@...lanox.com>, <leon@...nel.org>,
        <michael.chan@...adcom.com>, <vishal@...lsio.com>,
        <jeffrey.t.kirsher@...el.com>, <idosch@...lanox.com>,
        <aelior@...vell.com>, <peppe.cavallaro@...com>,
        <alexandre.torgue@...com>, <jhs@...atatu.com>,
        <xiyou.wangcong@...il.com>, <pablo@...filter.org>,
        <mlxsw@...lanox.com>
Subject: Re: [patch net-next v4 01/10] flow_offload: Introduce offload of HW
 stats type

On Mon, 9 Mar 2020 16:52:16 +0000 Edward Cree wrote:
> On 07/03/2020 11:40, Jiri Pirko wrote:
> > From: Jiri Pirko <jiri@...lanox.com>
> >
> > Initially, pass "ANY" (struct is zeroed) to the drivers as that is the
> > current implicit value coming down to flow_offload. Add a bool
> > indicating that entries have mixed HW stats type.
> >
> > Signed-off-by: Jiri Pirko <jiri@...lanox.com>
> > ---
> > v3->v4:
> > - fixed member alignment
> > v2->v3:
> > - moved to bitfield
> > - removed "mixed" bool
> > v1->v2:
> > - moved to actions
> > - add mixed bool
> > ---
> >  include/net/flow_offload.h | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> > index cd3510ac66b0..93d17f37e980 100644
> > --- a/include/net/flow_offload.h
> > +++ b/include/net/flow_offload.h
> > @@ -154,6 +154,8 @@ enum flow_action_mangle_base {
> >  	FLOW_ACT_MANGLE_HDR_TYPE_UDP,
> >  };
> >  
> > +#define FLOW_ACTION_HW_STATS_TYPE_ANY 0  
> I'm not quite sure why switching to a bit fieldapproach means these
>  haveto become #defines rather than enums...

Perhaps having enum defining the values could be argued...

> > +
> >  typedef void (*action_destr)(void *priv);
> >  
> >  struct flow_action_cookie {
> > @@ -168,6 +170,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie);
> >  
> >  struct flow_action_entry {
> >  	enum flow_action_id		id;
> > +	u8				hw_stats_type;  
> ... causing this to become a u8with nothing obviously preventing
>  a HW_STATS_TYPE bigger than 255 getting defined.
> An enum type seems safer.

...but using the type for fields which are very likely to contain
values from outside of the enumeration seems confusing, IMHO.

Driver author can understandably try to simply handle all the values 
in a switch statement and be unpleasantly surprised.