netdev - Re: [Intel-wired-lan] [next-queue PATCH v6 2/5] mqprio: Implement select_queue class

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 12 Oct 2017 09:16:40 -0700
From:   Jesus Sanchez-Palencia <jesus.sanchez-palencia@...el.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     Vinicius Costa Gomes <vinicius.gomes@...el.com>,
        Netdev <netdev@...r.kernel.org>,
        intel-wired-lan <intel-wired-lan@...ts.osuosl.org>,
        rodney.cummings@...com, andre.guedes@...el.com,
        Jiri Pirko <jiri@...nulli.us>, ivan.briano@...el.com,
        Richard Cochran <richardcochran@...il.com>, henrik@...tad.us,
        Jamal Hadi Salim <jhs@...atatu.com>, levipearson@...il.com,
        boon.leong.ong@...el.com, Cong Wang <xiyou.wangcong@...il.com>
Subject: Re: [Intel-wired-lan] [next-queue PATCH v6 2/5] mqprio: Implement
 select_queue class_ops



On 10/12/2017 09:09 AM, Alexander Duyck wrote:
> On Thu, Oct 12, 2017 at 8:59 AM, Jesus Sanchez-Palencia
> <jesus.sanchez-palencia@...el.com> wrote:
>> Hi Alex,
>>
>>
>> On 10/12/2017 08:21 AM, Alexander Duyck wrote:
>>> On Wed, Oct 11, 2017 at 5:54 PM, Vinicius Costa Gomes
>>> <vinicius.gomes@...el.com> wrote:
>>>> From: Jesus Sanchez-Palencia <jesus.sanchez-palencia@...el.com>
>>>>
>>>> When replacing a child qdisc from mqprio, tc_modify_qdisc() must fetch
>>>> the netdev_queue pointer that the current child qdisc is associated
>>>> with before creating the new qdisc.
>>>>
>>>> Currently, when using mqprio as root qdisc, the kernel will end up
>>>> getting the queue #0 pointer from the mqprio (root qdisc), which leaves
>>>> any new child qdisc with a possibly wrong netdev_queue pointer.
>>>>
>>>> Implementing the Qdisc_class_ops select_queue() on mqprio fixes this
>>>> issue and avoid an inconsistent state when child qdiscs are replaced.
>>>>
>>>> Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@...el.com>
>>>> ---
>>>>  net/sched/sch_mqprio.c | 7 +++++++
>>>>  1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
>>>> index 6bcdfe6e7b63..8c042ae323e3 100644
>>>> --- a/net/sched/sch_mqprio.c
>>>> +++ b/net/sched/sch_mqprio.c
>>>> @@ -396,6 +396,12 @@ static void mqprio_walk(struct Qdisc *sch, struct qdisc_walker *arg)
>>>>         }
>>>>  }
>>>>
>>>> +static struct netdev_queue *mqprio_select_queue(struct Qdisc *sch,
>>>> +                                               struct tcmsg *tcm)
>>>> +{
>>>> +       return mqprio_queue_get(sch, TC_H_MIN(tcm->tcm_parent));
>>>> +}
>>>> +
>>>
>>> So I was just comparing this against mq_selet_queue, and I was
>>> wondering why we are willing to return NULL here instead of just
>>> returning a pointer to the first Tx queue? I realize there is the fix
>>> in the first patch but it seems like if we are going to go that route
>>> then maybe we should update mq as well so that both of these qdiscs
>>> behave the same way. Either this should work like mq, or mq should
>>> work like this, but we shouldn't have them exposing different
>>> behaviors.
>>
>>
>> This was brought up by Cong Wang during the review of our v2. Based on my
>> understanding, the point I've made is that for mqprio the inner qdiscs are
>> always 'related' to one of the Tx netdev_queues per design. Returning any other
>> queue as a fallback seemed like going against that to me.
>>
>> I'm still inclined to say that we should keep this function as the patch is
>> proposing, thus either returning the correct netdev_queue for a given handle, or
>> NULL as a way to flag that something was 'wrong' with it. Returning queue #0 is
>> misleading in that sense, imho.
>>
>> As for aligning mq_select_queue() with this approach, if my reasoning behind
>> mqprio is correct and also applies to mq, I would be happy to send that fix as
>> part our v7.
>>
>> What do you think?
>>
>>
>> Thanks,
>> Jesus
> 
> I think it would be better to bring mq_select_queue in line with your
> fix. You could probably just add it to your first patch. That way if
> the user specifies a bad qdisc classid they don't get to just
> overwrite the qdisc on Tx queue 0.

Ok, I will send the fix then, but I'm just not sure if I'll send it together
with the patch fixing qdisc_alloc(). Looks like a change that should be together
with this one of after it, instead.

Anyhow, it will be fixed in our v7. Thanks!