diff mbox series

[v10,net-next,02/15] net/sched: act_api: increase action kind string length

Message ID 20240122194801.152658-3-jhs@mojatatu.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Introducing P4TC | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 5152 this patch: 5152
netdev/build_tools success Errors and warnings before: 2 this patch: 0
netdev/cc_maintainers success CCed 0 of 0 maintainers
netdev/build_clang success Errors and warnings before: 1278 this patch: 1278
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 5456 this patch: 5456
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 39 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-01-24--18-00 (tests: 520)

Commit Message

Jamal Hadi Salim Jan. 22, 2024, 7:47 p.m. UTC 
Increase action kind string length from IFNAMSIZ to 64

The new P4 actions, created via templates, will have longer names
of format: "pipeline_name/act_name". IFNAMSIZ is currently 16 and is most
of the times undersized for the above format.
So, to conform to this new format, we increase the maximum name length
and change its definition from IFNAMSIZ to ACTNAMSIZ to account for this extra
string (pipeline name) and the '/' character.

Co-developed-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
Co-developed-by: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
Reviewed-by: Vlad Buslov <vladbu@nvidia.com>
---
 include/net/act_api.h        | 2 +-
 include/uapi/linux/pkt_cls.h | 1 +
 net/sched/act_api.c          | 6 +++---
 3 files changed, 5 insertions(+), 4 deletions(-)

Comments

Marcelo Ricardo Leitner Feb. 1, 2024, 1:16 p.m. UTC | #1 
On Mon, Jan 22, 2024 at 02:47:48PM -0500, Jamal Hadi Salim wrote:
> @@ -1439,7 +1439,7 @@ tc_action_load_ops(struct net *net, struct nlattr *nla,
>  			NL_SET_ERR_MSG(extack, "TC action kind must be specified");
>  			return ERR_PTR(err);
>  		}
> -		if (nla_strscpy(act_name, kind, IFNAMSIZ) < 0) {
> +		if (nla_strscpy(act_name, kind, ACTNAMSIZ) < 0) {
>  			NL_SET_ERR_MSG(extack, "TC action name too long");
>  			return ERR_PTR(err);
>  		}

Subsquent lines here are:
        } else {
                if (strscpy(act_name, "police", IFNAMSIZ) < 0) {
		                                ^^^^^^^^
                        NL_SET_ERR_MSG(extack, "TC action name too long");

I know it won't make a difference in the end but it would be nice to
keep it consistent.
Marcelo Ricardo Leitner Feb. 1, 2024, 7:01 p.m. UTC | #2 
On Thu, Feb 01, 2024 at 05:16:44AM -0800, Marcelo Ricardo Leitner wrote:
> On Mon, Jan 22, 2024 at 02:47:48PM -0500, Jamal Hadi Salim wrote:
> > @@ -1439,7 +1439,7 @@ tc_action_load_ops(struct net *net, struct nlattr *nla,
> >  			NL_SET_ERR_MSG(extack, "TC action kind must be specified");
> >  			return ERR_PTR(err);
> >  		}
> > -		if (nla_strscpy(act_name, kind, IFNAMSIZ) < 0) {
> > +		if (nla_strscpy(act_name, kind, ACTNAMSIZ) < 0) {
> >  			NL_SET_ERR_MSG(extack, "TC action name too long");
> >  			return ERR_PTR(err);
> >  		}
>
> Subsquent lines here are:
>         } else {
>                 if (strscpy(act_name, "police", IFNAMSIZ) < 0) {
> 		                                ^^^^^^^^
>                         NL_SET_ERR_MSG(extack, "TC action name too long");
>
> I know it won't make a difference in the end but it would be nice to
> keep it consistent.
>

Despite this, please add my tag in the next iteration:

Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Jamal Hadi Salim Feb. 2, 2024, noon UTC | #3 
On Thu, Feb 1, 2024 at 8:16 AM Marcelo Ricardo Leitner
<mleitner@redhat.com> wrote:
>
> On Mon, Jan 22, 2024 at 02:47:48PM -0500, Jamal Hadi Salim wrote:
> > @@ -1439,7 +1439,7 @@ tc_action_load_ops(struct net *net, struct nlattr *nla,
> >                       NL_SET_ERR_MSG(extack, "TC action kind must be specified");
> >                       return ERR_PTR(err);
> >               }
> > -             if (nla_strscpy(act_name, kind, IFNAMSIZ) < 0) {
> > +             if (nla_strscpy(act_name, kind, ACTNAMSIZ) < 0) {
> >                       NL_SET_ERR_MSG(extack, "TC action name too long");
> >                       return ERR_PTR(err);
> >               }
>
> Subsquent lines here are:
>         } else {
>                 if (strscpy(act_name, "police", IFNAMSIZ) < 0) {
>                                                 ^^^^^^^^
>                         NL_SET_ERR_MSG(extack, "TC action name too long");
>
> I know it won't make a difference in the end but it would be nice to
> keep it consistent.

Agreed. It was an oversight. Will fix it in the next version.
Thanks for the rest of your reviews Marcelo!

cheers,
jamal
diff mbox series

Patch

diff --git a/include/net/act_api.h b/include/net/act_api.h
index ab28c2254..eb39a2101 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -106,7 +106,7 @@  typedef void (*tc_action_priv_destructor)(void *priv);
 struct tc_action_ops {
 	struct list_head head;
 	struct list_head p4_head;
-	char    kind[IFNAMSIZ];
+	char    kind[ACTNAMSIZ];
 	enum tca_id  id; /* identifier should match kind */
 	unsigned int	net_id;
 	size_t	size;
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index ea277039f..dd313a727 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -6,6 +6,7 @@ 
 #include <linux/pkt_sched.h>
 
 #define TC_COOKIE_MAX_SIZE 16
+#define ACTNAMSIZ 64
 
 /* Action attributes */
 enum {
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index e4a1b8f5a..2ff61be8e 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -476,7 +476,7 @@  static size_t tcf_action_shared_attrs_size(const struct tc_action *act)
 	rcu_read_unlock();
 
 	return  nla_total_size(0) /* action number nested */
-		+ nla_total_size(IFNAMSIZ) /* TCA_ACT_KIND */
+		+ nla_total_size(ACTNAMSIZ) /* TCA_ACT_KIND */
 		+ cookie_len /* TCA_ACT_COOKIE */
 		+ nla_total_size(sizeof(struct nla_bitfield32)) /* TCA_ACT_HW_STATS */
 		+ nla_total_size(0) /* TCA_ACT_STATS nested */
@@ -1424,7 +1424,7 @@  tc_action_load_ops(struct net *net, struct nlattr *nla,
 	bool police = flags & TCA_ACT_FLAGS_POLICE;
 	struct nlattr *tb[TCA_ACT_MAX + 1];
 	struct tc_action_ops *a_o;
-	char act_name[IFNAMSIZ];
+	char act_name[ACTNAMSIZ];
 	struct nlattr *kind;
 	int err;
 
@@ -1439,7 +1439,7 @@  tc_action_load_ops(struct net *net, struct nlattr *nla,
 			NL_SET_ERR_MSG(extack, "TC action kind must be specified");
 			return ERR_PTR(err);
 		}
-		if (nla_strscpy(act_name, kind, IFNAMSIZ) < 0) {
+		if (nla_strscpy(act_name, kind, ACTNAMSIZ) < 0) {
 			NL_SET_ERR_MSG(extack, "TC action name too long");
 			return ERR_PTR(err);
 		}