-----Original Message----- From: Guillaume Nault gnault@redhat.com Sent: środa, 29 marca 2023 17:26 To: Drewek, Wojciech wojciech.drewek@intel.com Cc: Andrea Righi andrea.righi@canonical.com; David S. Miller davem@davemloft.net; Eric Dumazet edumazet@google.com; Jakub Kicinski kuba@kernel.org; Paolo Abeni pabeni@redhat.com; Shuah Khan shuah@kernel.org; netdev@vger.kernel.org; linux-kselftest@vger.kernel.org; linux-kernel@vger.kernel.org Subject: Re: selftests: net: l2tp.sh regression starting with 6.1-rc1

On Wed, Mar 29, 2023 at 02:16:37PM +0000, Drewek, Wojciech wrote:

...

Hi,

Modifying UAPI was not a good idea although the patch should not break userspace (related discussion [1]). We could revert this patch with one additional change (include l2tp.h in net/sched/cls_flower.c) but then again, modifying UAPI. This patch was mostly cosmetic anyway. Second option is to try to fix the automatic load. I'm not an expert but I think MODULE_ALIAS_NET_PF_PROTO macro is somehow responsible for that. I noticed some comments saying that "__stringify doesn't like enums" (this macro is using _stringify) and my patch defined IPPROTO_L2TP in enum. We can just replace IPPROTO_L2TP with 115 (where this macro is used) in order to fix this. I'm going to give it a try and will let you know.

Yes, the modules aliases now have symbolic names:

$ modinfo l2tp_ip l2tp_ip6 | grep alias alias: net-pf-2-proto-IPPROTO_L2TP alias: net-pf-2-proto-2-type-IPPROTO_L2TP alias: net-pf-10-proto-IPPROTO_L2TP alias: net-pf-10-proto-2-type-IPPROTO_L2TP

Therefore, 'request_module("net-pf-%d-proto-%d-type-%d")' can't find them.

My personal preference is for the second option: fix module loading by using plain numbers in MODULE_ALIAS_*. We can always keep the symbolic names in comments.

---- >8 ----

diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c index 4db5a554bdbd..afe94a390ef0 100644 --- a/net/l2tp/l2tp_ip.c +++ b/net/l2tp/l2tp_ip.c @@ -680,5 +680,5 @@ MODULE_VERSION("1.0"); /* Use the value of SOCK_DGRAM (2) directory, because __stringify doesn't like

• enums

*/ -MODULE_ALIAS_NET_PF_PROTO_TYPE(PF_INET, 2, IPPROTO_L2TP); -MODULE_ALIAS_NET_PF_PROTO(PF_INET, IPPROTO_L2TP); +MODULE_ALIAS_NET_PF_PROTO_TYPE(PF_INET, 2, 115 /* IPPROTO_L2TP */); +MODULE_ALIAS_NET_PF_PROTO(PF_INET, 115 /* IPPROTO_L2TP */); diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c index 2478aa60145f..65d106b41951 100644 --- a/net/l2tp/l2tp_ip6.c +++ b/net/l2tp/l2tp_ip6.c @@ -809,5 +809,5 @@ MODULE_VERSION("1.0"); /* Use the value of SOCK_DGRAM (2) directory, because __stringify doesn't like

• enums

*/ -MODULE_ALIAS_NET_PF_PROTO_TYPE(PF_INET6, 2, IPPROTO_L2TP); -MODULE_ALIAS_NET_PF_PROTO(PF_INET6, IPPROTO_L2TP); +MODULE_ALIAS_NET_PF_PROTO_TYPE(PF_INET6, 2, 115 /* IPPROTO_L2TP */); +MODULE_ALIAS_NET_PF_PROTO(PF_INET6, 115 /* IPPROTO_L2TP */);

Btw, am I blind or the alias with type was wrong the whole time? pf goes first, then proto and type at the end according to the definition of MODULE_ALIAS_NET_PF_PROTO_TYPE and here type (2) is 2nd and proto (115) is 3rd

On Wed, Mar 29, 2023 at 06:52:20PM +0200, Guillaume Nault wrote:

On Wed, Mar 29, 2023 at 03:39:13PM +0000, Drewek, Wojciech wrote:

...

...

-----Original Message----- -MODULE_ALIAS_NET_PF_PROTO_TYPE(PF_INET6, 2, IPPROTO_L2TP); -MODULE_ALIAS_NET_PF_PROTO(PF_INET6, IPPROTO_L2TP); +MODULE_ALIAS_NET_PF_PROTO_TYPE(PF_INET6, 2, 115 /* IPPROTO_L2TP */); +MODULE_ALIAS_NET_PF_PROTO(PF_INET6, 115 /* IPPROTO_L2TP */);

Btw, am I blind or the alias with type was wrong the whole time? pf goes first, then proto and type at the end according to the definition of MODULE_ALIAS_NET_PF_PROTO_TYPE and here type (2) is 2nd and proto (115) is 3rd

You're not blind :). The MODULE_ALIAS_NET_PF_PROTO_TYPE(...) is indeed wrong. Auto-loading the l2tp_ip and l2tp_ip6 modules only worked because of the extra MODULE_ALIAS_NET_PF_PROTO() declaration (as inet_create() and inet6_create() fallback to "net-pf-%d-proto-%d" if "net-pf-%d-proto-%d-type-%d" fails).

At this point I think using 115 directly is probably the best solution, that is also what we do already with SOCK_DGRAM, but I would just update the comment up above, instead of adding the inline comments.

Something like this maybe:

---

From: Andrea Righi andrea.righi@canonical.com Subject: [PATCH] l2tp: generate correct module alias strings

Commit 65b32f801bfb ("uapi: move IPPROTO_L2TP to in.h") moved the definition of IPPROTO_L2TP from a define to an enum, but since __stringify doesn't work properly with enums, we ended up breaking the modalias strings for the l2tp modules:

$ modinfo l2tp_ip l2tp_ip6 | grep alias alias: net-pf-2-proto-IPPROTO_L2TP alias: net-pf-2-proto-2-type-IPPROTO_L2TP alias: net-pf-10-proto-IPPROTO_L2TP alias: net-pf-10-proto-2-type-IPPROTO_L2TP

Use the resolved number directly in MODULE_ALIAS_*() macros (as we already do with SOCK_DGRAM) to fix the alias strings:

$ modinfo l2tp_ip l2tp_ip6 | grep alias alias: net-pf-2-proto-115 alias: net-pf-2-proto-115-type-2 alias: net-pf-10-proto-115 alias: net-pf-10-proto-115-type-2

Moreover, fix the ordering of the parameters passed to MODULE_ALIAS_NET_PF_PROTO_TYPE() by switching proto and type.

Fixes: 65b32f801bfb ("uapi: move IPPROTO_L2TP to in.h") Signed-off-by: Guillaume Nault gnault@redhat.com Signed-off-by: Andrea Righi andrea.righi@canonical.com --- net/l2tp/l2tp_ip.c | 8 ++++---- net/l2tp/l2tp_ip6.c | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c index 4db5a554bdbd..41a74fc84ca1 100644 --- a/net/l2tp/l2tp_ip.c +++ b/net/l2tp/l2tp_ip.c @@ -677,8 +677,8 @@ MODULE_AUTHOR("James Chapman jchapman@katalix.com"); MODULE_DESCRIPTION("L2TP over IP"); MODULE_VERSION("1.0");

-/* Use the value of SOCK_DGRAM (2) directory, because __stringify doesn't like - * enums +/* Use the values of SOCK_DGRAM (2) as type and IPPROTO_L2TP (115) as protocol, + * because __stringify doesn't like enums */ -MODULE_ALIAS_NET_PF_PROTO_TYPE(PF_INET, 2, IPPROTO_L2TP); -MODULE_ALIAS_NET_PF_PROTO(PF_INET, IPPROTO_L2TP); +MODULE_ALIAS_NET_PF_PROTO_TYPE(PF_INET, 115, 2); +MODULE_ALIAS_NET_PF_PROTO(PF_INET, 115); diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c index 2478aa60145f..5137ea1861ce 100644 --- a/net/l2tp/l2tp_ip6.c +++ b/net/l2tp/l2tp_ip6.c @@ -806,8 +806,8 @@ MODULE_AUTHOR("Chris Elston celston@katalix.com"); MODULE_DESCRIPTION("L2TP IP encapsulation for IPv6"); MODULE_VERSION("1.0");

-/* Use the value of SOCK_DGRAM (2) directory, because __stringify doesn't like - * enums +/* Use the values of SOCK_DGRAM (2) as type and IPPROTO_L2TP (115) as protocol, + * because __stringify doesn't like enums */ -MODULE_ALIAS_NET_PF_PROTO_TYPE(PF_INET6, 2, IPPROTO_L2TP); -MODULE_ALIAS_NET_PF_PROTO(PF_INET6, IPPROTO_L2TP); +MODULE_ALIAS_NET_PF_PROTO_TYPE(PF_INET6, 115, 2); +MODULE_ALIAS_NET_PF_PROTO(PF_INET6, 115);