Created attachment 1296176 [details]
reproducer - many qdiscs + testmode

extend the reproducer to try it on multiple qdiscs. Using testmode =1, so that res.class is initialized to 0x100, before invoking the filter, Iobserved the NULL pointer dereference using the following queue disciplines:

- HTB
- CBQ
- DRR
- QFQ
- HFSC

At least in the htb case, the patch just causes htb to classify on a random classid (depending on the previous memory contents).

The correct way is to behave like there was no filter, i.e. invoke continue if no classification happened. The problem here is the current API does not pass that information out of the filters. This will require deeper changes, I'm afraid.

Created attachment 1322756 [details]
patch for upstream 4.13

(In reply to Jiri Benc from comment #6)
> At least in the htb case, the patch just causes htb to classify on a random
> classid (depending on the previous memory contents).
> 
> The correct way is to behave like there was no filter, i.e. invoke continue
> if no classification happened. The problem here is the current API does not
> pass that information out of the filters. This will require deeper changes,
> I'm afraid.

hello Jiri,

'matchall' seems to be the only tc filter that does not do anything to the tcf_result. Other filters assign their own configured values (tipically class = 0 and classid = < 32-bit id configured from userspace>. Maybe it's enough to add a single line in mall_classify(), before the actions are invoked, where the configured values are copied into tcf_result.

I tried this one-line patch with reproducer and observed no more crashes _ and accidentally saw that the 'classid' parameter of the matchall man page (http://man7.org/linux/man-pages/man8/tc-matchall.8.html) is then used in htb_classify.

Do you think this is acceptable upstream?
thank you in advance,
regards
-- 
davide

(In reply to Davide Caratti from comment #7)
> Do you think this is acceptable upstream?

It does not solve the problem, it just hides it. I don't think that's what upstream wants.

Btw, I spoke about this with Jiri Pirko and Jamal but I'm quite sure they forgot by now :-) I'm adding Jiri and opening the bug, there's nothing secret in here.

(In reply to Jiri Benc from comment #8)
> (In reply to Davide Caratti from comment #7)
> > Do you think this is acceptable upstream?
> 
> It does not solve the problem, it just hides it. I don't think that's what
> upstream wants.

Jiri,

looking at the sources, it seems that 'matchall' never worked as classifier since the beginning (i.e. it doesn't provide a classid value in the output, like other classifiers do). 

But this does not look intentional, otherwise the UAPI wouldn't expose TCA_MATCHALL_CLASSID as a control for the 'matchall' classifier (and there wouldn't be a reference of it in the man page). Currently, whatever 32bit number is written in tb[TCA_MATCHALL_CLASSID], the kernel ignores it; IMHO this is worth fixing (or we should deprecate it), because similar controls for other filters (i.e TCA_U32_CLASSID, TCA_TCINDEX_CLASSID or TCA_BASIC_CLASSID) are functional.

That said, users might expect that cls_matchall skips classification, and jumps to the next filter as you propose in comment #6, in case the TCA_MATCHALL_CLASSID is not specified. This behavior can be also made generic so that it can be implemented in other classifiers (like u32, or tc_index, or others).

Any opinion?
thank you in advance,
--
davide

Created attachment 1325902 [details]
patch for upstream 4.13 (updated message)

hi Jiri (Benc),

I looked at other classifiers code: all of them write to tcf_result when the filter matches, even when the 'classid' parameter is not specified by the user (in this last case, the struct is filled with zero).
To let 'matchall' avoid classification and jump to the next filter (like you suggest in comment #6) , we can make mall_classify() return a negative value (e.g. -1, TC_ACT_UNSPEC): same as what 'tcindex' classifier does when it's configured with 'pass_on' option and the packet doesn't match (see http://man7.org/linux/man-pages/man8/tc-tcindex.8.html).

Anyway, this is a change of behavior for 'matchall', we should at least make it configurable and documented, i.e. specifying a keyword in iproute2 that makes it jump to the next filter (I'm testing a patch that implements this).
And, before changing 'matchall' behavior, we should at least fix the current behavior - where the user can configure 'classid' but matchall doesn't care of that value.

Unless you have objections, I will post the attached patch (where I tried to describe the problem more clearly) targeting net.git, and a proposal for adding configurable 'pass_on' to cls_matchall, targeting net-next.git.

thank you in advance for any feedback,
regards
-- 
davide

bz1494554 has been filed as followup of comment #6 and conmment #11

https://www.spinics.net/lists/netdev/msg455010.html

Patch(es) committed on kernel repository and an interim kernel build is undergoing testing

Patch(es) available on kernel-3.10.0-742.el7

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2018:1062