[LTP] [PATCH] tcindex01: Pass if the tcindex module is blacklisted

[LTP] [PATCH] tcindex01: Pass if the tcindex module is blacklisted

[Martin Doucha]

The tcindex01 test currently fails if the tcindex module is enabled
in kernel config but cannot be autoloaded. Some distros chose
to blacklist the module rather than remove it completely, thus
check for autoload failure and pass in that case.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---
 testcases/cve/tcindex01.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/testcases/cve/tcindex01.c b/testcases/cve/tcindex01.c
index 70e5639f1..07239f9c0 100644
--- a/testcases/cve/tcindex01.c
+++ b/testcases/cve/tcindex01.c
@@ -106,8 +106,19 @@ static void run(void)
 	NETDEV_ADD_QDISC(DEVNAME, AF_UNSPEC, TC_H_ROOT, qd_handle, "htb",
 		qd_config);
 	NETDEV_ADD_TRAFFIC_CLASS(DEVNAME, qd_handle, clsid, "htb", cls_config);
-	NETDEV_ADD_TRAFFIC_FILTER(DEVNAME, qd_handle, 10, ETH_P_IP, 1,
-		"tcindex", f_config);
+	ret = tst_netdev_add_traffic_filter(__FILE__, __LINE__, 0, DEVNAME,
+		qd_handle, 10, ETH_P_IP, 1, "tcindex", f_config);
+	TST_ERR = tst_netlink_errno;
+
+	if (!ret && TST_ERR == ENOENT) {
+		tst_res(TPASS | TTERRNO,
+			"tcindex module is blacklisted or unavailable");
+		return;
+	}
+
+	if (!ret)
+		tst_brk(TBROK | TTERRNO, "Cannot add tcindex filter");
+
 	NETDEV_REMOVE_TRAFFIC_FILTER(DEVNAME, qd_handle, 10, ETH_P_IP,
 		1, "tcindex");
 	ret = tst_netdev_add_traffic_filter(__FILE__, __LINE__, 0, DEVNAME,
-- 
2.44.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] tcindex01: Pass if the tcindex module is blacklisted

[Petr Vorel]

Hi Martin,

> The tcindex01 test currently fails if the tcindex module is enabled
> in kernel config but cannot be autoloaded. Some distros chose
> to blacklist the module rather than remove it completely, thus
> check for autoload failure and pass in that case.

> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> ---
>  testcases/cve/tcindex01.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)

> diff --git a/testcases/cve/tcindex01.c b/testcases/cve/tcindex01.c
> index 70e5639f1..07239f9c0 100644
> --- a/testcases/cve/tcindex01.c
> +++ b/testcases/cve/tcindex01.c
> @@ -106,8 +106,19 @@ static void run(void)
>  	NETDEV_ADD_QDISC(DEVNAME, AF_UNSPEC, TC_H_ROOT, qd_handle, "htb",
>  		qd_config);
>  	NETDEV_ADD_TRAFFIC_CLASS(DEVNAME, qd_handle, clsid, "htb", cls_config);
> -	NETDEV_ADD_TRAFFIC_FILTER(DEVNAME, qd_handle, 10, ETH_P_IP, 1,
> -		"tcindex", f_config);
> +	ret = tst_netdev_add_traffic_filter(__FILE__, __LINE__, 0, DEVNAME,

nit: we now don't use NETDEV_ADD_TRAFFIC_FILTER() macro any more. I guess it can
stay because you sooner or later will use it.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

> +		qd_handle, 10, ETH_P_IP, 1, "tcindex", f_config);
> +	TST_ERR = tst_netlink_errno;
Out of curriosity, I suppose you save tst_netlink_errno to TST_ERR because it
would be overwritten later in other LTP netlink API functions.

> +
> +	if (!ret && TST_ERR == ENOENT) {
> +		tst_res(TPASS | TTERRNO,
> +			"tcindex module is blacklisted or unavailable");
> +		return;
> +	}

Kind regards,
Petr
> +
> +	if (!ret)
> +		tst_brk(TBROK | TTERRNO, "Cannot add tcindex filter");
> +
>  	NETDEV_REMOVE_TRAFFIC_FILTER(DEVNAME, qd_handle, 10, ETH_P_IP,
>  		1, "tcindex");
>  	ret = tst_netdev_add_traffic_filter(__FILE__, __LINE__, 0, DEVNAME,

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] tcindex01: Pass if the tcindex module is blacklisted

[Cyril Hrubis]

Hi!
> > diff --git a/testcases/cve/tcindex01.c b/testcases/cve/tcindex01.c
> > index 70e5639f1..07239f9c0 100644
> > --- a/testcases/cve/tcindex01.c
> > +++ b/testcases/cve/tcindex01.c
> > @@ -106,8 +106,19 @@ static void run(void)
> >  	NETDEV_ADD_QDISC(DEVNAME, AF_UNSPEC, TC_H_ROOT, qd_handle, "htb",
> >  		qd_config);
> >  	NETDEV_ADD_TRAFFIC_CLASS(DEVNAME, qd_handle, clsid, "htb", cls_config);
> > -	NETDEV_ADD_TRAFFIC_FILTER(DEVNAME, qd_handle, 10, ETH_P_IP, 1,
> > -		"tcindex", f_config);
> > +	ret = tst_netdev_add_traffic_filter(__FILE__, __LINE__, 0, DEVNAME,

I do not like that much that we add the __FILE__ and __LINE__ into the
test by hand. Maybe just add another macro
NETDEV_ADD_TRAFIC_FILTER_RET() so that we don't have to write these into
the testcases?

> nit: we now don't use NETDEV_ADD_TRAFFIC_FILTER() macro any more. I guess it can
> stay because you sooner or later will use it.
> 
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> 
> > +		qd_handle, 10, ETH_P_IP, 1, "tcindex", f_config);
> > +	TST_ERR = tst_netlink_errno;
> Out of curriosity, I suppose you save tst_netlink_errno to TST_ERR because it
> would be overwritten later in other LTP netlink API functions.

Because he wants to print it with TTERRNO later.

> > +
> > +	if (!ret && TST_ERR == ENOENT) {
> > +		tst_res(TPASS | TTERRNO,
> > +			"tcindex module is blacklisted or unavailable");
> > +		return;
> > +	}

I guess that our .needs_drivers does not take blacklists into account,
otherwise we could have just added tcindex into .needs_drivers.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] tcindex01: Pass if the tcindex module is blacklisted

[Petr Vorel]

> Hi!
> > > diff --git a/testcases/cve/tcindex01.c b/testcases/cve/tcindex01.c
> > > index 70e5639f1..07239f9c0 100644
> > > --- a/testcases/cve/tcindex01.c
> > > +++ b/testcases/cve/tcindex01.c
> > > @@ -106,8 +106,19 @@ static void run(void)
> > >  	NETDEV_ADD_QDISC(DEVNAME, AF_UNSPEC, TC_H_ROOT, qd_handle, "htb",
> > >  		qd_config);
> > >  	NETDEV_ADD_TRAFFIC_CLASS(DEVNAME, qd_handle, clsid, "htb", cls_config);
> > > -	NETDEV_ADD_TRAFFIC_FILTER(DEVNAME, qd_handle, 10, ETH_P_IP, 1,
> > > -		"tcindex", f_config);
> > > +	ret = tst_netdev_add_traffic_filter(__FILE__, __LINE__, 0, DEVNAME,

> I do not like that much that we add the __FILE__ and __LINE__ into the
> test by hand. Maybe just add another macro
> NETDEV_ADD_TRAFIC_FILTER_RET() so that we don't have to write these into
> the testcases?

> > nit: we now don't use NETDEV_ADD_TRAFFIC_FILTER() macro any more. I guess it can
> > stay because you sooner or later will use it.

> > Reviewed-by: Petr Vorel <pvorel@suse.cz>

> > > +		qd_handle, 10, ETH_P_IP, 1, "tcindex", f_config);
> > > +	TST_ERR = tst_netlink_errno;
> > Out of curriosity, I suppose you save tst_netlink_errno to TST_ERR because it
> > would be overwritten later in other LTP netlink API functions.

> Because he wants to print it with TTERRNO later.

> > > +
> > > +	if (!ret && TST_ERR == ENOENT) {
> > > +		tst_res(TPASS | TTERRNO,
> > > +			"tcindex module is blacklisted or unavailable");
> > > +		return;
> > > +	}

> I guess that our .needs_drivers does not take blacklists into account,
> otherwise we could have just added tcindex into .needs_drivers.

That reminds me .modprobe_module WIP patchset. I was not able to continue with
it, also I'm still gathering what is needed, I was not sure if it's needed to
add it or it'd be possible to enhance .needs_drivers. Also, I'd be great to
collect these few tests with non-standard requirements into a single ticket.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] tcindex01: Pass if the tcindex module is blacklisted

[Martin Doucha]

On 15. 05. 24 14:22, Petr Vorel wrote:
> That reminds me .modprobe_module WIP patchset. I was not able to continue with
> it, also I'm still gathering what is needed, I was not sure if it's needed to
> add it or it'd be possible to enhance .needs_drivers. Also, I'd be great to
> collect these few tests with non-standard requirements into a single ticket.

For this test, we definitely don't want the LTP library to modprobe the 
module. Because explicit modprobe would succeed despite blacklist.

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] tcindex01: Pass if the tcindex module is blacklisted

[Petr Vorel]

> On 15. 05. 24 14:22, Petr Vorel wrote:
> > That reminds me .modprobe_module WIP patchset. I was not able to continue with
> > it, also I'm still gathering what is needed, I was not sure if it's needed to
> > add it or it'd be possible to enhance .needs_drivers. Also, I'd be great to
> > collect these few tests with non-standard requirements into a single ticket.

> For this test, we definitely don't want the LTP library to modprobe the
> module. Because explicit modprobe would succeed despite blacklist.

Thanks for info. Does it mean that test requires to tcindex be loaded
automatically via that traffic filter? I.e. simple modprobe tcindex
would spoil testing? Your way testing ENOENT is obviously correct (and working
now without library modifications), but modprobe for detection followed by
"modprobe -r" could also work (non-existing code).

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] tcindex01: Pass if the tcindex module is blacklisted

[Petr Vorel]

> Hi!
> > > diff --git a/testcases/cve/tcindex01.c b/testcases/cve/tcindex01.c
> > > index 70e5639f1..07239f9c0 100644
> > > --- a/testcases/cve/tcindex01.c
> > > +++ b/testcases/cve/tcindex01.c
> > > @@ -106,8 +106,19 @@ static void run(void)
> > >  	NETDEV_ADD_QDISC(DEVNAME, AF_UNSPEC, TC_H_ROOT, qd_handle, "htb",
> > >  		qd_config);
> > >  	NETDEV_ADD_TRAFFIC_CLASS(DEVNAME, qd_handle, clsid, "htb", cls_config);
> > > -	NETDEV_ADD_TRAFFIC_FILTER(DEVNAME, qd_handle, 10, ETH_P_IP, 1,
> > > -		"tcindex", f_config);
> > > +	ret = tst_netdev_add_traffic_filter(__FILE__, __LINE__, 0, DEVNAME,

> I do not like that much that we add the __FILE__ and __LINE__ into the
> test by hand. Maybe just add another macro
> NETDEV_ADD_TRAFIC_FILTER_RET() so that we don't have to write these into
> the testcases?

IMHO it makes sense.

> > nit: we now don't use NETDEV_ADD_TRAFFIC_FILTER() macro any more. I guess it can
> > stay because you sooner or later will use it.

> > Reviewed-by: Petr Vorel <pvorel@suse.cz>

> > > +		qd_handle, 10, ETH_P_IP, 1, "tcindex", f_config);
> > > +	TST_ERR = tst_netlink_errno;
> > Out of curriosity, I suppose you save tst_netlink_errno to TST_ERR because it
> > would be overwritten later in other LTP netlink API functions.

> Because he wants to print it with TTERRNO later.

+1

> > > +
> > > +	if (!ret && TST_ERR == ENOENT) {
> > > +		tst_res(TPASS | TTERRNO,
> > > +			"tcindex module is blacklisted or unavailable");

Why not TCONF? We are testing if removing tcindex does not cause bug,
right?

> > > +		return;
> > > +	}

> I guess that our .needs_drivers does not take blacklists into account,
> otherwise we could have just added tcindex into .needs_drivers.

Yes, it only searches modules.builtin and modules.dep.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] tcindex01: Pass if the tcindex module is blacklisted

[Martin Doucha]

On 15. 05. 24 15:03, Petr Vorel wrote:
>>>> +
>>>> +	if (!ret && TST_ERR == ENOENT) {
>>>> +		tst_res(TPASS | TTERRNO,
>>>> +			"tcindex module is blacklisted or unavailable");
> 
> Why not TCONF? We are testing if removing tcindex does not cause bug,
> right?

Blacklisting the module is intended as a partial CVE fix so TPASS is 
more appropriate than TCONF.

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] tcindex01: Pass if the tcindex module is blacklisted

[Petr Vorel]

> On 15. 05. 24 15:03, Petr Vorel wrote:
> > > > > +
> > > > > +	if (!ret && TST_ERR == ENOENT) {
> > > > > +		tst_res(TPASS | TTERRNO,
> > > > > +			"tcindex module is blacklisted or unavailable");

> > Why not TCONF? We are testing if removing tcindex does not cause bug,
> > right?

> Blacklisting the module is intended as a partial CVE fix so TPASS is more
> appropriate than TCONF.

Thanks for info. Indeed it makes sense. Maybe it's just me, who didn't see it as
obvious when looking into CVE-2023-1829 description. But OK "We recommend
upgrading past commit 8c710f75256bb3cf05ac7b1672c82b92c43f3d28." + your
description makes it clear that some distros just preferred blacklist
configuration instead of backporting the removal commit.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp