diff mbox series

[v4,12/12] selftests: ip_local_port_range: use XFAIL instead of SKIP

Message ID 20240229005920.2407409-13-kuba@kernel.org (mailing list archive)
State Accepted
Commit c05bf0e933129dbb71d057949d90531b03462538
Headers show
Series selftests: kselftest_harness: support using xfail | expand

Commit Message

Jakub Kicinski Feb. 29, 2024, 12:59 a.m. UTC
SCTP does not support IP_LOCAL_PORT_RANGE and we know it,
so use XFAIL instead of SKIP.

Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/net/ip_local_port_range.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Jakub Sitnicki Feb. 29, 2024, 8:19 p.m. UTC | #1
On Wed, Feb 28, 2024 at 04:59 PM -08, Jakub Kicinski wrote:
> SCTP does not support IP_LOCAL_PORT_RANGE and we know it,
> so use XFAIL instead of SKIP.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/testing/selftests/net/ip_local_port_range.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/net/ip_local_port_range.c b/tools/testing/selftests/net/ip_local_port_range.c
> index 6ebd58869a63..193b82745fd8 100644
> --- a/tools/testing/selftests/net/ip_local_port_range.c
> +++ b/tools/testing/selftests/net/ip_local_port_range.c
> @@ -365,9 +365,6 @@ TEST_F(ip_local_port_range, late_bind)
>  	__u32 range;
>  	__u16 port;
>  
> -	if (variant->so_protocol == IPPROTO_SCTP)
> -		SKIP(return, "SCTP doesn't support IP_BIND_ADDRESS_NO_PORT");
> -
>  	fd = socket(variant->so_domain, variant->so_type, 0);
>  	ASSERT_GE(fd, 0) TH_LOG("socket failed");
>  
> @@ -414,6 +411,9 @@ TEST_F(ip_local_port_range, late_bind)
>  	ASSERT_TRUE(!err) TH_LOG("close failed");
>  }
>  
> +XFAIL_ADD(ip_local_port_range, ip4_stcp, late_bind);
> +XFAIL_ADD(ip_local_port_range, ip6_stcp, late_bind);
> +
>  TEST_F(ip_local_port_range, get_port_range)
>  {
>  	__u16 lo, hi;

[wrt our earlier discussion off-list]

You were right, this test succeeds if I delete SKIP for SCTP.
Turns out IP_LOCAL_PORT_RANGE works for SCTP out of the box after all.

What I didn't notice earlier is that sctp_setsockopt() delegates to
ip_setsockopt() when level != SOL_SCTP.

CC'ing Marcelo & Xin, to confirm that this isn't a problem.
Xin Long Feb. 29, 2024, 11:25 p.m. UTC | #2
On Thu, Feb 29, 2024 at 3:27 PM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>
> On Wed, Feb 28, 2024 at 04:59 PM -08, Jakub Kicinski wrote:
> > SCTP does not support IP_LOCAL_PORT_RANGE and we know it,
> > so use XFAIL instead of SKIP.
> >
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > ---
> >  tools/testing/selftests/net/ip_local_port_range.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/ip_local_port_range.c b/tools/testing/selftests/net/ip_local_port_range.c
> > index 6ebd58869a63..193b82745fd8 100644
> > --- a/tools/testing/selftests/net/ip_local_port_range.c
> > +++ b/tools/testing/selftests/net/ip_local_port_range.c
> > @@ -365,9 +365,6 @@ TEST_F(ip_local_port_range, late_bind)
> >       __u32 range;
> >       __u16 port;
> >
> > -     if (variant->so_protocol == IPPROTO_SCTP)
> > -             SKIP(return, "SCTP doesn't support IP_BIND_ADDRESS_NO_PORT");
> > -
> >       fd = socket(variant->so_domain, variant->so_type, 0);
> >       ASSERT_GE(fd, 0) TH_LOG("socket failed");
> >
> > @@ -414,6 +411,9 @@ TEST_F(ip_local_port_range, late_bind)
> >       ASSERT_TRUE(!err) TH_LOG("close failed");
> >  }
> >
> > +XFAIL_ADD(ip_local_port_range, ip4_stcp, late_bind);
> > +XFAIL_ADD(ip_local_port_range, ip6_stcp, late_bind);
> > +
> >  TEST_F(ip_local_port_range, get_port_range)
> >  {
> >       __u16 lo, hi;
>
> [wrt our earlier discussion off-list]
>
> You were right, this test succeeds if I delete SKIP for SCTP.
> Turns out IP_LOCAL_PORT_RANGE works for SCTP out of the box after all.
>
> What I didn't notice earlier is that sctp_setsockopt() delegates to
> ip_setsockopt() when level != SOL_SCTP.
>
> CC'ing Marcelo & Xin, to confirm that this isn't a problem.
Yes, SCTP supports ip_local_port_range by calling
inet_sk_get_local_port_range() in sctp_get_port(), similar to TCP/UDP.
Jakub Sitnicki March 1, 2024, 10:40 a.m. UTC | #3
On Thu, Feb 29, 2024 at 06:25 PM -05, Xin Long wrote:
> On Thu, Feb 29, 2024 at 3:27 PM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>>
>> On Wed, Feb 28, 2024 at 04:59 PM -08, Jakub Kicinski wrote:
>> > SCTP does not support IP_LOCAL_PORT_RANGE and we know it,
>> > so use XFAIL instead of SKIP.
>> >
>> > Reviewed-by: Kees Cook <keescook@chromium.org>
>> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>> > ---
>> >  tools/testing/selftests/net/ip_local_port_range.c | 6 +++---
>> >  1 file changed, 3 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/tools/testing/selftests/net/ip_local_port_range.c b/tools/testing/selftests/net/ip_local_port_range.c
>> > index 6ebd58869a63..193b82745fd8 100644
>> > --- a/tools/testing/selftests/net/ip_local_port_range.c
>> > +++ b/tools/testing/selftests/net/ip_local_port_range.c
>> > @@ -365,9 +365,6 @@ TEST_F(ip_local_port_range, late_bind)
>> >       __u32 range;
>> >       __u16 port;
>> >
>> > -     if (variant->so_protocol == IPPROTO_SCTP)
>> > -             SKIP(return, "SCTP doesn't support IP_BIND_ADDRESS_NO_PORT");
>> > -
>> >       fd = socket(variant->so_domain, variant->so_type, 0);
>> >       ASSERT_GE(fd, 0) TH_LOG("socket failed");
>> >
>> > @@ -414,6 +411,9 @@ TEST_F(ip_local_port_range, late_bind)
>> >       ASSERT_TRUE(!err) TH_LOG("close failed");
>> >  }
>> >
>> > +XFAIL_ADD(ip_local_port_range, ip4_stcp, late_bind);
>> > +XFAIL_ADD(ip_local_port_range, ip6_stcp, late_bind);
>> > +
>> >  TEST_F(ip_local_port_range, get_port_range)
>> >  {
>> >       __u16 lo, hi;
>>
>> [wrt our earlier discussion off-list]
>>
>> You were right, this test succeeds if I delete SKIP for SCTP.
>> Turns out IP_LOCAL_PORT_RANGE works for SCTP out of the box after all.
>>
>> What I didn't notice earlier is that sctp_setsockopt() delegates to
>> ip_setsockopt() when level != SOL_SCTP.
>>
>> CC'ing Marcelo & Xin, to confirm that this isn't a problem.
> Yes, SCTP supports ip_local_port_range by calling
> inet_sk_get_local_port_range() in sctp_get_port(), similar to TCP/UDP.

Well, that's embarassing.

I see that I've updated sctp stack to use inet_sk_get_local_port_range()
in 91d0b78c5177 ("inet: Add IP_LOCAL_PORT_RANGE socket option").

Thanks for confirming, Xin.

It's clearly an overside on my side. That SKIP in tests should have
never been there. I will send a fixup.
diff mbox series

Patch

diff --git a/tools/testing/selftests/net/ip_local_port_range.c b/tools/testing/selftests/net/ip_local_port_range.c
index 6ebd58869a63..193b82745fd8 100644
--- a/tools/testing/selftests/net/ip_local_port_range.c
+++ b/tools/testing/selftests/net/ip_local_port_range.c
@@ -365,9 +365,6 @@  TEST_F(ip_local_port_range, late_bind)
 	__u32 range;
 	__u16 port;
 
-	if (variant->so_protocol == IPPROTO_SCTP)
-		SKIP(return, "SCTP doesn't support IP_BIND_ADDRESS_NO_PORT");
-
 	fd = socket(variant->so_domain, variant->so_type, 0);
 	ASSERT_GE(fd, 0) TH_LOG("socket failed");
 
@@ -414,6 +411,9 @@  TEST_F(ip_local_port_range, late_bind)
 	ASSERT_TRUE(!err) TH_LOG("close failed");
 }
 
+XFAIL_ADD(ip_local_port_range, ip4_stcp, late_bind);
+XFAIL_ADD(ip_local_port_range, ip6_stcp, late_bind);
+
 TEST_F(ip_local_port_range, get_port_range)
 {
 	__u16 lo, hi;