| Message ID | 20240515094753.1072-1-mdoucha@suse.cz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tcindex01: Pass if the tcindex module is blacklisted | expand |
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,
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.
> 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
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.
> 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
> 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
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.
> 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
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,
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(-)