[PATCH 0/5] Fix event generation for actions batch Add/Delete mode

[PATCH 0/5] Fix event generation for actions batch Add/Delete mode

[Roman Mashak]

When adding or deleting a batch of entries, the kernel sends upto
TCA_ACT_MAX_PRIO entries in an event to user space. However it does not
consider that the action sizes may vary and require different skb sizes.

For example :

% cat tc-batch.sh
#!/bin/bash
TC="sudo /mnt/iproute2.git/tc/tc"

$TC actions flush action gact
for i in `seq 1 $1`;
do
   cmd="action pass index $i "
   args=$args$cmd
done
$TC actions add $args
%
% ./tc-batch.sh 32
Error: Failed to fill netlink attributes while deleting TC action.
We have an error talking to the kernel
%

This patchset introduces new callback in tc_action_ops, which calculates
the action size, and passes size to tcf_add_notify()/tcf_del_notify(). The
patch fixes act_gact and act_police, and the rest of actions will be
updated in the follow-up patches.

Roman Mashak (5):
  net sched actions: routines to calculate common TLVs size
  net sched actions: add new tc_action_ops callback
  net sched actions: calculate add/delete event message size.
  net sched actions: implement get_fill_size routine in act_gact
  net sched actions: implement get_fill_size routine in act_police

 include/net/act_api.h  |  6 ++++-
 net/sched/act_api.c    | 62 +++++++++++++++++++++++++++++++++++++++++++-------
 net/sched/act_gact.c   | 18 +++++++++++++++
 net/sched/act_police.c |  9 ++++++++
 net/sched/cls_api.c    |  3 ++-
 5 files changed, 88 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH 1/5] net sched actions: routines to calculate common TLVs size

[Roman Mashak]

Introduce routine to calculate size of the common tc netlink attributes,
and another helper routine to get the full message size including netlink
header and service header.

Signed-off-by: Roman Mashak <mrv@mojatatu.com>
---
 net/sched/act_api.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 1f65d6a..acac92a 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -109,6 +109,33 @@ int __tcf_idr_release(struct tc_action *p, bool bind, bool strict)
 }
 EXPORT_SYMBOL(__tcf_idr_release);
 
+static size_t tcf_action_shared_attrs_size(const struct tc_action *act)
+{
+	u32 cookie_len = 0;
+
+	if (act->act_cookie)
+		cookie_len = nla_total_size(act->act_cookie->len);
+
+	return	nla_total_size(0) /* action number nested */
+		+ nla_total_size(IFNAMSIZ) /* TCA_ACT_KIND */
+		+ cookie_len /* TCA_ACT_COOKIE */
+		+ nla_total_size(0) /* TCA_ACT_STATS nested */
+		/* TCA_STATS_BASIC */
+		+ nla_total_size_64bit(sizeof(struct gnet_stats_basic))
+		/* TCA_STATS_QUEUE */
+		+ nla_total_size_64bit(sizeof(struct gnet_stats_queue))
+		+ nla_total_size(0) /* TCA_OPTIONS nested */
+		+ nla_total_size(sizeof(struct tcf_t));	/* TCA_GACT_TM */
+}
+
+static size_t tcf_action_full_attrs_size(size_t sz)
+{
+	return NLMSG_HDRLEN			/* struct nlmsghdr */
+		+ sizeof(struct tcamsg)
+		+ nla_total_size(0)		/* TCA_ACT_TAB nested */
+		+ sz;
+}
+
 static int tcf_dump_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
 			   struct netlink_callback *cb)
 {
-- 
2.7.4

[PATCH 2/5] net sched actions: add new tc_action_ops callback

[Roman Mashak]

Add a new callback in tc_action_ops, it will be needed by the tc actions
to compute its size when a ADD/DELETE notification message is constructed.
This routine has to take into account optional/variable size TLVs specific
per action.

Signed-off-by: Roman Mashak <mrv@mojatatu.com>
---
 include/net/act_api.h | 1 +
 net/sched/act_api.c   | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 9c2f226..0a56465 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -97,6 +97,7 @@ struct tc_action_ops {
 			const struct tc_action_ops *,
 			struct netlink_ext_ack *);
 	void	(*stats_update)(struct tc_action *, u64, u32, u64);
+	size_t  (*get_fill_size)(const struct tc_action *act);
 	struct net_device *(*get_dev)(const struct tc_action *a);
 };
 
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index acac92a..6f3307f 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -136,6 +136,14 @@ static size_t tcf_action_full_attrs_size(size_t sz)
 		+ sz;
 }
 
+static size_t tcf_action_fill_size(const struct tc_action *act)
+{
+	if (act->ops->get_fill_size)
+		return act->ops->get_fill_size(act) +
+			tcf_action_shared_attrs_size(act);
+	return 0;
+}
+
 static int tcf_dump_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
 			   struct netlink_callback *cb)
 {
-- 
2.7.4

[PATCH 3/5] net sched actions: calculate add/delete event message size

[Roman Mashak]

Update add/delete action logic to have the size for event messages,
the size is passed to tcf_add_notify() and tcf_del_notify().

Signed-off-by: Roman Mashak <mrv@mojatatu.com>
---
 include/net/act_api.h |  3 ++-
 net/sched/act_api.c   | 26 ++++++++++++++++++--------
 net/sched/cls_api.c   |  3 ++-
 3 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 0a56465..e0a9c20 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -167,7 +167,8 @@ int tcf_action_exec(struct sk_buff *skb, struct tc_action **actions,
 		    int nr_actions, struct tcf_result *res);
 int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
 		    struct nlattr *est, char *name, int ovr, int bind,
-		    struct list_head *actions, struct netlink_ext_ack *extack);
+		    struct list_head *actions, size_t *attr_size,
+		    struct netlink_ext_ack *extack);
 struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 				    struct nlattr *nla, struct nlattr *est,
 				    char *name, int ovr, int bind,
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 6f3307f..097ca07 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -776,10 +776,12 @@ static void cleanup_a(struct list_head *actions, int ovr)
 
 int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
 		    struct nlattr *est, char *name, int ovr, int bind,
-		    struct list_head *actions, struct netlink_ext_ack *extack)
+		    struct list_head *actions, size_t *attr_size,
+		    struct netlink_ext_ack *extack)
 {
 	struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
 	struct tc_action *act;
+	size_t sz = 0;
 	int err;
 	int i;
 
@@ -795,11 +797,14 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
 			goto err;
 		}
 		act->order = i;
+		sz += tcf_action_fill_size(act);
 		if (ovr)
 			act->tcfa_refcnt++;
 		list_add_tail(&act->list, actions);
 	}
 
+	*attr_size = tcf_action_full_attrs_size(sz);
+
 	/* Remove the temp refcnt which was necessary to protect against
 	 * destroying an existing action which was being replaced
 	 */
@@ -1029,12 +1034,13 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
 
 static int
 tcf_del_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
-	       u32 portid, struct netlink_ext_ack *extack)
+	       u32 portid, size_t attr_size, struct netlink_ext_ack *extack)
 {
 	int ret;
 	struct sk_buff *skb;
 
-	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
+	skb = alloc_skb(attr_size <= NLMSG_GOODSIZE ? NLMSG_GOODSIZE : attr_size,
+			GFP_KERNEL);
 	if (!skb)
 		return -ENOBUFS;
 
@@ -1067,6 +1073,7 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 	int i, ret;
 	struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
 	struct tc_action *act;
+	size_t attr_size = 0;
 	LIST_HEAD(actions);
 
 	ret = nla_parse_nested(tb, TCA_ACT_MAX_PRIO, nla, NULL, extack);
@@ -1088,13 +1095,14 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 			goto err;
 		}
 		act->order = i;
+		attr_size += tcf_action_fill_size(act);
 		list_add_tail(&act->list, &actions);
 	}
 
 	if (event == RTM_GETACTION)
 		ret = tcf_get_notify(net, portid, n, &actions, event, extack);
 	else { /* delete */
-		ret = tcf_del_notify(net, n, &actions, portid, extack);
+		ret = tcf_del_notify(net, n, &actions, portid, attr_size, extack);
 		if (ret)
 			goto err;
 		return ret;
@@ -1107,12 +1115,13 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 
 static int
 tcf_add_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
-	       u32 portid, struct netlink_ext_ack *extack)
+	       u32 portid, size_t attr_size, struct netlink_ext_ack *extack)
 {
 	struct sk_buff *skb;
 	int err = 0;
 
-	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
+	skb = alloc_skb(attr_size <= NLMSG_GOODSIZE ? NLMSG_GOODSIZE : attr_size,
+			GFP_KERNEL);
 	if (!skb)
 		return -ENOBUFS;
 
@@ -1134,15 +1143,16 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
 			  struct nlmsghdr *n, u32 portid, int ovr,
 			  struct netlink_ext_ack *extack)
 {
+	size_t attr_size = 0;
 	int ret = 0;
 	LIST_HEAD(actions);
 
 	ret = tcf_action_init(net, NULL, nla, NULL, NULL, ovr, 0, &actions,
-			      extack);
+			      &attr_size, extack);
 	if (ret)
 		return ret;
 
-	return tcf_add_notify(net, n, &actions, portid, extack);
+	return tcf_add_notify(net, n, &actions, portid, attr_size, extack);
 }
 
 static u32 tcaa_root_flags_allowed = TCA_FLAG_LARGE_DUMP_ON;
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 19f9f42..ec5fe8e 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1433,6 +1433,7 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
 #ifdef CONFIG_NET_CLS_ACT
 	{
 		struct tc_action *act;
+		size_t attr_size = 0;
 
 		if (exts->police && tb[exts->police]) {
 			act = tcf_action_init_1(net, tp, tb[exts->police],
@@ -1450,7 +1451,7 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
 
 			err = tcf_action_init(net, tp, tb[exts->action],
 					      rate_tlv, NULL, ovr, TCA_ACT_BIND,
-					      &actions, extack);
+					      &actions, &attr_size, extack);
 			if (err)
 				return err;
 			list_for_each_entry(act, &actions, list)
-- 
2.7.4

[PATCH 4/5] net sched actions: implement get_fill_size routine in act_gact

[Roman Mashak]

Signed-off-by: Roman Mashak <mrv@mojatatu.com>
---
 net/sched/act_gact.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index 7456325..ee775ac 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -217,6 +217,22 @@ static int tcf_gact_search(struct net *net, struct tc_action **a, u32 index,
 	return tcf_idr_search(tn, a, index);
 }
 
+static size_t tcf_gact_get_fill_size(const struct tc_action *act)
+{
+#ifdef CONFIG_GACT_PROB
+	struct tcf_gact *gact = to_gact(act);
+	int prob_len = 0;
+
+	if (gact->tcfg_ptype)
+		prob_len = nla_total_size(sizeof(struct tc_gact_p));
+#endif
+	return	nla_total_size(sizeof(struct tc_gact)) /* TCA_GACT_PARMS */
+#ifdef CONFIG_GACT_PROB
+		+ prob_len /* TCA_GACT_PROB */
+#endif
+		;
+}
+
 static struct tc_action_ops act_gact_ops = {
 	.kind		=	"gact",
 	.type		=	TCA_ACT_GACT,
@@ -227,6 +243,7 @@ static struct tc_action_ops act_gact_ops = {
 	.init		=	tcf_gact_init,
 	.walk		=	tcf_gact_walker,
 	.lookup		=	tcf_gact_search,
+	.get_fill_size	=	tcf_gact_get_fill_size,
 	.size		=	sizeof(struct tcf_gact),
 };
 
-- 
2.7.4

[PATCH 5/5] net sched actions: implement get_fill_size routine in act_police

[Roman Mashak]

Signed-off-by: Roman Mashak <mrv@mojatatu.com>
---
 net/sched/act_police.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 51fe4fe..d4b4b15 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -314,6 +314,13 @@ static int tcf_police_search(struct net *net, struct tc_action **a, u32 index,
 	return tcf_idr_search(tn, a, index);
 }
 
+static size_t tcf_police_get_fill_size(const struct tc_action *act)
+{
+	return nla_total_size(sizeof(struct tc_police)) /* TCA_POLICE_TBF */
+		+ nla_total_size(sizeof(u32)) /* TCA_POLICE_RESULT */
+		+ nla_total_size(sizeof(u32)); /* TCA_POLICE_AVRATE */
+}
+
 MODULE_AUTHOR("Alexey Kuznetsov");
 MODULE_DESCRIPTION("Policing actions");
 MODULE_LICENSE("GPL");
@@ -327,6 +334,7 @@ static struct tc_action_ops act_police_ops = {
 	.init		=	tcf_act_police_init,
 	.walk		=	tcf_act_police_walker,
 	.lookup		=	tcf_police_search,
+	.get_fill_size	=	tcf_police_get_fill_size,
 	.size		=	sizeof(struct tcf_police),
 };
 
-- 
2.7.4

Re: [PATCH 4/5] net sched actions: implement get_fill_size routine in act_gact

[Marcelo Ricardo Leitner]

On Thu, Mar 01, 2018 at 01:45:30PM -0500, Roman Mashak wrote:
> Signed-off-by: Roman Mashak <mrv@mojatatu.com>
> ---
>  net/sched/act_gact.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
> index 7456325..ee775ac 100644
> --- a/net/sched/act_gact.c
> +++ b/net/sched/act_gact.c
> @@ -217,6 +217,22 @@ static int tcf_gact_search(struct net *net, struct tc_action **a, u32 index,
>  	return tcf_idr_search(tn, a, index);
>  }
>  
> +static size_t tcf_gact_get_fill_size(const struct tc_action *act)
> +{
> +#ifdef CONFIG_GACT_PROB
> +	struct tcf_gact *gact = to_gact(act);
> +	int prob_len = 0;
> +
> +	if (gact->tcfg_ptype)
> +		prob_len = nla_total_size(sizeof(struct tc_gact_p));
> +#endif
> +	return	nla_total_size(sizeof(struct tc_gact)) /* TCA_GACT_PARMS */
> +#ifdef CONFIG_GACT_PROB
> +		+ prob_len /* TCA_GACT_PROB */
> +#endif
> +		;
> +}

This is quite bad to the eye. What about:

static size_t tcf_gact_get_fill_size(const struct tc_action *act)
{
	size_t sz = nla_total_size(sizeof(struct tc_gact)); /* TCA_GACT_PARMS */

#ifdef CONFIG_GACT_PROB
	if (to_gact(act)->tcfg_ptype)
		sz += nla_total_size(sizeof(struct tc_gact_p)); /* TCA_GACT_PROB */
#endif

	return sz;
}

Should be the same, bug easier to read.

> +
>  static struct tc_action_ops act_gact_ops = {
>  	.kind		=	"gact",
>  	.type		=	TCA_ACT_GACT,
> @@ -227,6 +243,7 @@ static struct tc_action_ops act_gact_ops = {
>  	.init		=	tcf_gact_init,
>  	.walk		=	tcf_gact_walker,
>  	.lookup		=	tcf_gact_search,
> +	.get_fill_size	=	tcf_gact_get_fill_size,
>  	.size		=	sizeof(struct tcf_gact),
>  };
>  
> -- 
> 2.7.4
> 

Re: [PATCH 4/5] net sched actions: implement get_fill_size routine in act_gact

[Roman Mashak]

Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> writes:

> On Thu, Mar 01, 2018 at 01:45:30PM -0500, Roman Mashak wrote:
>> Signed-off-by: Roman Mashak <mrv@mojatatu.com>
>> ---
>>  net/sched/act_gact.c | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>> 
>> diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
>> index 7456325..ee775ac 100644
>> --- a/net/sched/act_gact.c
>> +++ b/net/sched/act_gact.c
>> @@ -217,6 +217,22 @@ static int tcf_gact_search(struct net *net, struct tc_action **a, u32 index,
>>  	return tcf_idr_search(tn, a, index);
>>  }
>>  
>> +static size_t tcf_gact_get_fill_size(const struct tc_action *act)
>> +{
>> +#ifdef CONFIG_GACT_PROB
>> +	struct tcf_gact *gact = to_gact(act);
>> +	int prob_len = 0;
>> +
>> +	if (gact->tcfg_ptype)
>> +		prob_len = nla_total_size(sizeof(struct tc_gact_p));
>> +#endif
>> +	return	nla_total_size(sizeof(struct tc_gact)) /* TCA_GACT_PARMS */
>> +#ifdef CONFIG_GACT_PROB
>> +		+ prob_len /* TCA_GACT_PROB */
>> +#endif
>> +		;
>> +}
>
> This is quite bad to the eye. What about:
>
> static size_t tcf_gact_get_fill_size(const struct tc_action *act)
> {
> 	size_t sz = nla_total_size(sizeof(struct tc_gact)); /* TCA_GACT_PARMS */
>
> #ifdef CONFIG_GACT_PROB
> 	if (to_gact(act)->tcfg_ptype)
> 		sz += nla_total_size(sizeof(struct tc_gact_p)); /* TCA_GACT_PROB */
> #endif
>
> 	return sz;
> }
>
> Should be the same, bug easier to read.

Agree, I will send v2.