2023-11-21 17:57:41

by Michal Koutný

[permalink] [raw]
Subject: [PATCH] net/sched: cls: Load net classifier modules via alias

The classifier modules may be loaded lazily without user's awareness and
control. Add respective aliases to modules and request them under these
aliases so that modprobe's blacklisting mechanism works also for
classifier modules. (The same pattern exists e.g. for filesystem
modules.)

Original module names remain unchanged.

Signed-off-by: Michal Koutný <[email protected]>
---
include/net/pkt_cls.h | 1 +
net/sched/cls_api.c | 2 +-
net/sched/cls_basic.c | 1 +
net/sched/cls_bpf.c | 1 +
net/sched/cls_cgroup.c | 1 +
net/sched/cls_flow.c | 1 +
net/sched/cls_flower.c | 1 +
net/sched/cls_fw.c | 1 +
net/sched/cls_matchall.c | 1 +
net/sched/cls_route.c | 1 +
net/sched/cls_u32.c | 1 +
11 files changed, 11 insertions(+), 1 deletion(-)

This is primarily for TC subsystem maintainers where the
request_module() resides but Cc list is large because of touches in
various classifier modules.

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index a76c9171db0e..424b4f889feb 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -24,6 +24,7 @@ struct tcf_walker {

int register_tcf_proto_ops(struct tcf_proto_ops *ops);
void unregister_tcf_proto_ops(struct tcf_proto_ops *ops);
+#define MODULE_ALIAS_TCF(kind) MODULE_ALIAS("tcf-" __stringify(kind))

struct tcf_block_ext_info {
enum flow_block_binder_type binder_type;
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 1976bd163986..02fdcceee083 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -257,7 +257,7 @@ tcf_proto_lookup_ops(const char *kind, bool rtnl_held,
#ifdef CONFIG_MODULES
if (rtnl_held)
rtnl_unlock();
- request_module("cls_%s", kind);
+ request_module("tcf-%s", kind);
if (rtnl_held)
rtnl_lock();
ops = __tcf_proto_lookup_ops(kind);
diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
index a1f56931330c..a3500ac7fc1a 100644
--- a/net/sched/cls_basic.c
+++ b/net/sched/cls_basic.c
@@ -328,6 +328,7 @@ static struct tcf_proto_ops cls_basic_ops __read_mostly = {
.bind_class = basic_bind_class,
.owner = THIS_MODULE,
};
+MODULE_ALIAS_TCF("basic");

static int __init init_basic(void)
{
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 382c7a71f81f..8d57ac155c0c 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -693,6 +693,7 @@ static struct tcf_proto_ops cls_bpf_ops __read_mostly = {
.dump = cls_bpf_dump,
.bind_class = cls_bpf_bind_class,
};
+MODULE_ALIAS_TCF("bpf");

static int __init cls_bpf_init_mod(void)
{
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 7ee8dbf49ed0..0ded7d79894c 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -209,6 +209,7 @@ static struct tcf_proto_ops cls_cgroup_ops __read_mostly = {
.dump = cls_cgroup_dump,
.owner = THIS_MODULE,
};
+MODULE_ALIAS_TCF("cgroup");

static int __init init_cgroup_cls(void)
{
diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index 6ab317b48d6c..2806aa1254e1 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -702,6 +702,7 @@ static struct tcf_proto_ops cls_flow_ops __read_mostly = {
.walk = flow_walk,
.owner = THIS_MODULE,
};
+MODULE_ALIAS_TCF("flow");

static int __init cls_flow_init(void)
{
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index e5314a31f75a..739e09e0fa57 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -3633,6 +3633,7 @@ static struct tcf_proto_ops cls_fl_ops __read_mostly = {
.owner = THIS_MODULE,
.flags = TCF_PROTO_OPS_DOIT_UNLOCKED,
};
+MODULE_ALIAS_TCF("flower");

static int __init cls_fl_init(void)
{
diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index afc534ee0a18..86c833885a2d 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -433,6 +433,7 @@ static struct tcf_proto_ops cls_fw_ops __read_mostly = {
.bind_class = fw_bind_class,
.owner = THIS_MODULE,
};
+MODULE_ALIAS_TCF("fw");

static int __init init_fw(void)
{
diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index c4ed11df6254..21ba73978c6a 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -398,6 +398,7 @@ static struct tcf_proto_ops cls_mall_ops __read_mostly = {
.bind_class = mall_bind_class,
.owner = THIS_MODULE,
};
+MODULE_ALIAS_TCF("matchall");

static int __init cls_mall_init(void)
{
diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
index 12a505db4183..a4701c0752df 100644
--- a/net/sched/cls_route.c
+++ b/net/sched/cls_route.c
@@ -671,6 +671,7 @@ static struct tcf_proto_ops cls_route4_ops __read_mostly = {
.bind_class = route4_bind_class,
.owner = THIS_MODULE,
};
+MODULE_ALIAS_TCF("route");

static int __init init_route4(void)
{
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index d5bdfd4a7655..a969adbd7423 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -1453,6 +1453,7 @@ static struct tcf_proto_ops cls_u32_ops __read_mostly = {
.bind_class = u32_bind_class,
.owner = THIS_MODULE,
};
+MODULE_ALIAS_TCF("u32");

static int __init init_u32(void)
{
--
2.42.1


2023-11-21 22:38:07

by Jamal Hadi Salim

[permalink] [raw]
Subject: Re: [PATCH] net/sched: cls: Load net classifier modules via alias

On Tue, Nov 21, 2023 at 12:56 PM Michal Koutný <[email protected]> wrote:
>
> The classifier modules may be loaded lazily without user's awareness and
> control. Add respective aliases to modules and request them under these
> aliases so that modprobe's blacklisting mechanism works also for
> classifier modules. (The same pattern exists e.g. for filesystem
> modules.)
>

Hi Michal,
Dumb question: What's speacial about the "tcf- '' that makes it work
better for filtering than existing "cls_" prefix? What about actions
(prefix "act_") etc?

cheers,
jamal

> Original module names remain unchanged.
>
> Signed-off-by: Michal Koutný <[email protected]>
> ---
> include/net/pkt_cls.h | 1 +
> net/sched/cls_api.c | 2 +-
> net/sched/cls_basic.c | 1 +
> net/sched/cls_bpf.c | 1 +
> net/sched/cls_cgroup.c | 1 +
> net/sched/cls_flow.c | 1 +
> net/sched/cls_flower.c | 1 +
> net/sched/cls_fw.c | 1 +
> net/sched/cls_matchall.c | 1 +
> net/sched/cls_route.c | 1 +
> net/sched/cls_u32.c | 1 +
> 11 files changed, 11 insertions(+), 1 deletion(-)
>
> This is primarily for TC subsystem maintainers where the
> request_module() resides but Cc list is large because of touches in
> various classifier modules.
>
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index a76c9171db0e..424b4f889feb 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -24,6 +24,7 @@ struct tcf_walker {
>
> int register_tcf_proto_ops(struct tcf_proto_ops *ops);
> void unregister_tcf_proto_ops(struct tcf_proto_ops *ops);
> +#define MODULE_ALIAS_TCF(kind) MODULE_ALIAS("tcf-" __stringify(kind))
>
> struct tcf_block_ext_info {
> enum flow_block_binder_type binder_type;
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 1976bd163986..02fdcceee083 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -257,7 +257,7 @@ tcf_proto_lookup_ops(const char *kind, bool rtnl_held,
> #ifdef CONFIG_MODULES
> if (rtnl_held)
> rtnl_unlock();
> - request_module("cls_%s", kind);
> + request_module("tcf-%s", kind);
> if (rtnl_held)
> rtnl_lock();
> ops = __tcf_proto_lookup_ops(kind);
> diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
> index a1f56931330c..a3500ac7fc1a 100644
> --- a/net/sched/cls_basic.c
> +++ b/net/sched/cls_basic.c
> @@ -328,6 +328,7 @@ static struct tcf_proto_ops cls_basic_ops __read_mostly = {
> .bind_class = basic_bind_class,
> .owner = THIS_MODULE,
> };
> +MODULE_ALIAS_TCF("basic");
>
> static int __init init_basic(void)
> {
> diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
> index 382c7a71f81f..8d57ac155c0c 100644
> --- a/net/sched/cls_bpf.c
> +++ b/net/sched/cls_bpf.c
> @@ -693,6 +693,7 @@ static struct tcf_proto_ops cls_bpf_ops __read_mostly = {
> .dump = cls_bpf_dump,
> .bind_class = cls_bpf_bind_class,
> };
> +MODULE_ALIAS_TCF("bpf");
>
> static int __init cls_bpf_init_mod(void)
> {
> diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
> index 7ee8dbf49ed0..0ded7d79894c 100644
> --- a/net/sched/cls_cgroup.c
> +++ b/net/sched/cls_cgroup.c
> @@ -209,6 +209,7 @@ static struct tcf_proto_ops cls_cgroup_ops __read_mostly = {
> .dump = cls_cgroup_dump,
> .owner = THIS_MODULE,
> };
> +MODULE_ALIAS_TCF("cgroup");
>
> static int __init init_cgroup_cls(void)
> {
> diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
> index 6ab317b48d6c..2806aa1254e1 100644
> --- a/net/sched/cls_flow.c
> +++ b/net/sched/cls_flow.c
> @@ -702,6 +702,7 @@ static struct tcf_proto_ops cls_flow_ops __read_mostly = {
> .walk = flow_walk,
> .owner = THIS_MODULE,
> };
> +MODULE_ALIAS_TCF("flow");
>
> static int __init cls_flow_init(void)
> {
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index e5314a31f75a..739e09e0fa57 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -3633,6 +3633,7 @@ static struct tcf_proto_ops cls_fl_ops __read_mostly = {
> .owner = THIS_MODULE,
> .flags = TCF_PROTO_OPS_DOIT_UNLOCKED,
> };
> +MODULE_ALIAS_TCF("flower");
>
> static int __init cls_fl_init(void)
> {
> diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
> index afc534ee0a18..86c833885a2d 100644
> --- a/net/sched/cls_fw.c
> +++ b/net/sched/cls_fw.c
> @@ -433,6 +433,7 @@ static struct tcf_proto_ops cls_fw_ops __read_mostly = {
> .bind_class = fw_bind_class,
> .owner = THIS_MODULE,
> };
> +MODULE_ALIAS_TCF("fw");
>
> static int __init init_fw(void)
> {
> diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
> index c4ed11df6254..21ba73978c6a 100644
> --- a/net/sched/cls_matchall.c
> +++ b/net/sched/cls_matchall.c
> @@ -398,6 +398,7 @@ static struct tcf_proto_ops cls_mall_ops __read_mostly = {
> .bind_class = mall_bind_class,
> .owner = THIS_MODULE,
> };
> +MODULE_ALIAS_TCF("matchall");
>
> static int __init cls_mall_init(void)
> {
> diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
> index 12a505db4183..a4701c0752df 100644
> --- a/net/sched/cls_route.c
> +++ b/net/sched/cls_route.c
> @@ -671,6 +671,7 @@ static struct tcf_proto_ops cls_route4_ops __read_mostly = {
> .bind_class = route4_bind_class,
> .owner = THIS_MODULE,
> };
> +MODULE_ALIAS_TCF("route");
>
> static int __init init_route4(void)
> {
> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> index d5bdfd4a7655..a969adbd7423 100644
> --- a/net/sched/cls_u32.c
> +++ b/net/sched/cls_u32.c
> @@ -1453,6 +1453,7 @@ static struct tcf_proto_ops cls_u32_ops __read_mostly = {
> .bind_class = u32_bind_class,
> .owner = THIS_MODULE,
> };
> +MODULE_ALIAS_TCF("u32");
>
> static int __init init_u32(void)
> {
> --
> 2.42.1
>

2023-11-22 10:41:28

by Michal Koutný

[permalink] [raw]
Subject: Re: [PATCH] net/sched: cls: Load net classifier modules via alias

On Tue, Nov 21, 2023 at 05:37:37PM -0500, Jamal Hadi Salim <[email protected]> wrote:
> What's speacial about the "tcf- '' that makes it work
> better for filtering than existing "cls_" prefix?

tcf-foo is an alias.
cls_foo is the canonical name of the kernel module.

request_module() + blacklist (as described in modprobe.d(5)) works only
when calling with the alias. The actual string is not important, being
an alias is the crux.

> What about actions (prefix "act_") etc?

I focused only on "cls_" for the first iteration. Do you want me to look
at other analogous loads?

Thanks,
Michal


Attachments:
(No filename) (610.00 B)
signature.asc (235.00 B)
Download all attachments

2023-11-22 15:34:30

by Jamal Hadi Salim

[permalink] [raw]
Subject: Re: [PATCH] net/sched: cls: Load net classifier modules via alias

On Wed, Nov 22, 2023 at 5:41 AM Michal Koutný <[email protected]> wrote:
>
> On Tue, Nov 21, 2023 at 05:37:37PM -0500, Jamal Hadi Salim <[email protected]> wrote:
> > What's speacial about the "tcf- '' that makes it work
> > better for filtering than existing "cls_" prefix?
>
> tcf-foo is an alias.
> cls_foo is the canonical name of the kernel module.
>
> request_module() + blacklist (as described in modprobe.d(5)) works only
> when calling with the alias. The actual string is not important, being
> an alias is the crux.
>

Thanks for the explanation.

> > What about actions (prefix "act_") etc?
>
> I focused only on "cls_" for the first iteration. Do you want me to look
> at other analogous loads?

Yes, look at act_ and sch_

cheers,
jamal
> Thanks,
> Michal

2023-11-22 16:12:21

by Michal Koutný

[permalink] [raw]
Subject: Re: [PATCH] net/sched: cls: Load net classifier modules via alias

On Wed, Nov 22, 2023 at 10:55:29AM -0500, Jamal Hadi Salim <[email protected]> wrote:
> Out of curiosity - how did you end up looking at this?

The trigger for this case was tcindex module -- removed in upstream, so
we don't want to have implicit autoloads for it while we retain it in
our distro.

The user has to modprobe explicitly or un-blacklist the module.

(For instance, we've been leveraging this for the fs-* modules due to
various support status of filesystem modules in SLE.)


Michal


Attachments:
(No filename) (510.00 B)
signature.asc (235.00 B)
Download all attachments

2023-11-22 16:13:09

by Jamal Hadi Salim

[permalink] [raw]
Subject: Re: [PATCH] net/sched: cls: Load net classifier modules via alias

On Wed, Nov 22, 2023 at 10:33 AM Jamal Hadi Salim <[email protected]> wrote:
>
> On Wed, Nov 22, 2023 at 5:41 AM Michal Koutný <[email protected]> wrote:
> >
> > On Tue, Nov 21, 2023 at 05:37:37PM -0500, Jamal Hadi Salim <[email protected]> wrote:
> > > What's speacial about the "tcf- '' that makes it work
> > > better for filtering than existing "cls_" prefix?
> >
> > tcf-foo is an alias.
> > cls_foo is the canonical name of the kernel module.
> >
> > request_module() + blacklist (as described in modprobe.d(5)) works only
> > when calling with the alias. The actual string is not important, being
> > an alias is the crux.
> >
>
> Thanks for the explanation.
>

Out of curiosity - how did you end up looking at this? Was there
someone who complained or is it just standard procedure to add aliases
to all modules and it was on a todo list somewhere?

cheers,
jamal

> > > What about actions (prefix "act_") etc?
> >
> > I focused only on "cls_" for the first iteration. Do you want me to look
> > at other analogous loads?
>
> Yes, look at act_ and sch_
>
> cheers,
> jamal
> > Thanks,
> > Michal