| 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
| ||
|
Message-ID: <DB5PR05MB189531C271EEE6BF7C71848EACF80@DB5PR05MB1895.eurprd05.prod.outlook.com>
Date: Wed, 7 Sep 2016 14:31:15 +0000
From: Nogah Frankel <nogahf@...lanox.com>
To: Roopa Prabhu <roopa@...ulusnetworks.com>,
Jiri Pirko <jiri@...nulli.us>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"davem@...emloft.net" <davem@...emloft.net>,
Ido Schimmel <idosch@...lanox.com>,
Elad Raz <eladr@...lanox.com>,
Yotam Gigi <yotamg@...lanox.com>,
Or Gerlitz <ogerlitz@...lanox.com>,
"nikolay@...ulusnetworks.com" <nikolay@...ulusnetworks.com>,
"linville@...driver.com" <linville@...driver.com>,
"tgraf@...g.ch" <tgraf@...g.ch>,
"gospo@...ulusnetworks.com" <gospo@...ulusnetworks.com>,
"sfeldma@...il.com" <sfeldma@...il.com>,
"sd@...asysnail.net" <sd@...asysnail.net>,
"Eran Ben Elisha" <eranbe@...lanox.com>,
"ast@...mgrid.com" <ast@...mgrid.com>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"hannes@...essinduktion.org" <hannes@...essinduktion.org>,
"f.fainelli@...il.com" <f.fainelli@...il.com>,
"dsa@...ulusnetworks.com" <dsa@...ulusnetworks.com>
Subject: RE: [patch net-next v7 2/3] net: core: Add offload stats to
if_stats_msg
> From: Roopa Prabhu [mailto:roopa@...ulusnetworks.com]
> Sent: Tuesday, September 06, 2016 6:20 PM
> To: Jiri Pirko <jiri@...nulli.us>
> Cc: netdev@...r.kernel.org; davem@...emloft.net; Nogah Frankel
> <nogahf@...lanox.com>; Ido Schimmel <idosch@...lanox.com>; Elad Raz
> <eladr@...lanox.com>; Yotam Gigi <yotamg@...lanox.com>; Or Gerlitz
> <ogerlitz@...lanox.com>; nikolay@...ulusnetworks.com;
> linville@...driver.com; tgraf@...g.ch; gospo@...ulusnetworks.com;
> sfeldma@...il.com; sd@...asysnail.net; Eran Ben Elisha
> <eranbe@...lanox.com>; ast@...mgrid.com; edumazet@...gle.com;
> hannes@...essinduktion.org; f.fainelli@...il.com;
> dsa@...ulusnetworks.com
> Subject: Re: [patch net-next v7 2/3] net: core: Add offload stats to if_stats_msg
>
> On 9/5/16, 10:18 AM, Jiri Pirko wrote:
> > From: Nogah Frankel <nogahf@...lanox.com>
> >
> > Add a nested attribute of offload stats to if_stats_msg
> > named IFLA_STATS_LINK_OFFLOAD_XSTATS.
> > Under it, add SW stats, meaning stats only per packets that went via
> > slowpath to the cpu, named IFLA_OFFLOAD_XSTATS_CPU_HIT.
> >
> > Signed-off-by: Nogah Frankel <nogahf@...lanox.com>
> > Signed-off-by: Jiri Pirko <jiri@...lanox.com>
> > ---
> > include/uapi/linux/if_link.h | 10 +++++
> > net/core/rtnetlink.c | 88
> ++++++++++++++++++++++++++++++++++++++++++--
> > 2 files changed, 94 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> > index 9bf3aec..4aaa2a1 100644
> > --- a/include/uapi/linux/if_link.h
> > +++ b/include/uapi/linux/if_link.h
> > @@ -826,6 +826,7 @@ enum {
> > IFLA_STATS_LINK_64,
> > IFLA_STATS_LINK_XSTATS,
> > IFLA_STATS_LINK_XSTATS_SLAVE,
> > + IFLA_STATS_LINK_OFFLOAD_XSTATS,
> > __IFLA_STATS_MAX,
> > };
> >
> > @@ -845,6 +846,15 @@ enum {
> > };
> > #define LINK_XSTATS_TYPE_MAX (__LINK_XSTATS_TYPE_MAX - 1)
> >
> > +/* These are stats embedded into IFLA_STATS_LINK_OFFLOAD_XSTATS */
> > +enum {
> > + IFLA_OFFLOAD_XSTATS_UNSPEC,
> > + IFLA_OFFLOAD_XSTATS_CPU_HIT, /* struct rtnl_link_stats64 */
> > + __IFLA_OFFLOAD_XSTATS_MAX
> > +};
> > +#define IFLA_OFFLOAD_XSTATS_MAX (__IFLA_OFFLOAD_XSTATS_MAX - 1)
> > +#define IFLA_OFFLOAD_XSTATS_FIRST (IFLA_OFFLOAD_XSTATS_UNSPEC +
> 1)
>
> not sure why we need a first in the uapi.
I'll move it to rtnetlink.
> > +
> > /* XDP section */
> >
> > enum {
> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> > index 1dfca1c..95eb131 100644
> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -3577,6 +3577,74 @@ static bool stats_attr_valid(unsigned int mask,
> int attrid, int idxattr)
> > (!idxattr || idxattr == attrid);
> > }
> >
> > +static int rtnl_get_offload_stats_attr_size(int attr_id)
> > +{
> > + switch (attr_id) {
> > + case IFLA_OFFLOAD_XSTATS_CPU_HIT:
> > + return sizeof(struct rtnl_link_stats64);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int rtnl_get_offload_stats(struct sk_buff *skb, struct net_device
> *dev)
> > +{
> > + struct nlattr *attr;
> > + int attr_id, size;
> > + void *attr_data;
> > + int err;
> > +
> > + if (!(dev->netdev_ops && dev->netdev_ops->ndo_has_offload_stats
> &&
> > + dev->netdev_ops->ndo_get_offload_stats))
> > + return 0;
> > +
> > + for (attr_id = IFLA_OFFLOAD_XSTATS_FIRST;
> > + attr_id <= IFLA_OFFLOAD_XSTATS_MAX; attr_id++) {
> > + size = rtnl_get_offload_stats_attr_size(attr_id);
> > + if (!size)
> > + continue;
> > +
> > + if (!dev->netdev_ops->ndo_has_offload_stats(attr_id))
> > + continue;
> > +
> > + attr = nla_reserve_64bit(skb, attr_id, size,
> > + IFLA_OFFLOAD_XSTATS_UNSPEC);
> > + if (!attr)
> > + return -EMSGSIZE;
>
> you need this 64bit alignment padding only for the specific nested attribute
> your driver is adding ?. The code seems to add it unconditionally for every
> attribute.
> But, I am guessing future attribute authors will need to adjust this somehow.
>
I think it's best to wait till we have more attributes to see how to implement it.
> > +
> > + attr_data = nla_data(attr);
> > + memset(attr_data, 0, size);
> > + err = dev->netdev_ops->ndo_get_offload_stats(attr_id, dev,
> > + attr_data);
> > + if (err)
> > + return err;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int rtnl_get_offload_stats_size(const struct net_device *dev)
> > +{
> > + int size = 0;
> > + int attr_id;
> > +
> > + if (!(dev->netdev_ops && dev->netdev_ops->ndo_has_offload_stats
> &&
> > + dev->netdev_ops->ndo_get_offload_stats))
> > + return nla_total_size(0);
> > +
> > + for (attr_id = IFLA_OFFLOAD_XSTATS_FIRST;
> > + attr_id <= IFLA_OFFLOAD_XSTATS_MAX; attr_id++) {
> > + if (!dev->netdev_ops->ndo_has_offload_stats(attr_id))
> > + continue;
> > +
> > + size += rtnl_get_offload_stats_attr_size(attr_id);
> > + }
> > +
> > + size += nla_total_size(0);
> > +
> > + return size;
> > +}
> > +
> > static int rtnl_fill_statsinfo(struct sk_buff *skb, struct net_device *dev,
> > int type, u32 pid, u32 seq, u32 change,
> > unsigned int flags, unsigned int filter_mask,
> > @@ -3586,6 +3654,7 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb,
> struct net_device *dev,
> > struct nlmsghdr *nlh;
> > struct nlattr *attr;
> > int s_prividx = *prividx;
> > + int err;
> >
> > ASSERT_RTNL();
> >
> > @@ -3614,8 +3683,6 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb,
> struct net_device *dev,
> > const struct rtnl_link_ops *ops = dev->rtnl_link_ops;
> >
> > if (ops && ops->fill_linkxstats) {
> > - int err;
> > -
> > *idxattr = IFLA_STATS_LINK_XSTATS;
> > attr = nla_nest_start(skb,
> > IFLA_STATS_LINK_XSTATS);
> > @@ -3639,8 +3706,6 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb,
> struct net_device *dev,
> > if (master)
> > ops = master->rtnl_link_ops;
> > if (ops && ops->fill_linkxstats) {
> > - int err;
> > -
> > *idxattr = IFLA_STATS_LINK_XSTATS_SLAVE;
> > attr = nla_nest_start(skb,
> > IFLA_STATS_LINK_XSTATS_SLAVE);
> > @@ -3655,6 +3720,18 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb,
> struct net_device *dev,
> > }
> > }
> >
> > + if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_OFFLOAD_XSTATS,
> > + *idxattr)) {
> > + attr = nla_nest_start(skb,
> IFLA_STATS_LINK_OFFLOAD_XSTATS);
> > + if (!attr)
> > + goto nla_put_failure;
> > +
> > + err = rtnl_get_offload_stats(skb, dev);
> > + nla_nest_end(skb, attr);
>
> seems like this can result in an empty nested attribute. better thing would be to
> cancel the nest if size from rtnl_get_offload_stats is zero ?
>
I'll fix it.
> > + if (err)
> > + goto nla_put_failure;
> > + }
> > +
> > nlmsg_end(skb, nlh);
> >
> > return 0;
> > @@ -3712,6 +3789,9 @@ static size_t if_nlmsg_stats_size(const struct
> net_device *dev,
> > }
> > }
> >
> > + if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_OFFLOAD_XSTATS,
> 0))
> > + size += rtnl_get_offload_stats_size(dev);
> > +
> > return size;
> > }
> >
Powered by blists - more mailing lists