linux-kernel - Re: [PATCH] xhci: fix unmatched num_trbs

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAATdQgDEwOZaBeXrsocF-b5-wfXgNGNhfaEBPsmhf7nefo55KQ@mail.gmail.com>
Date:   Mon, 19 Jul 2021 12:36:40 +0800
From:   Ikjoon Jang <ikjn@...omium.org>
To:     Mathias Nyman <mathias.nyman@...ux.intel.com>
Cc:     linux-usb@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Mathias Nyman <mathias.nyman@...el.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] xhci: fix unmatched num_trbs_free

Hi Mathias,

On Fri, Jul 16, 2021 at 8:54 PM Mathias Nyman
<mathias.nyman@...ux.intel.com> wrote:
>
> On 8.7.2021 11.43, Ikjoon Jang wrote:
> > When unlinked urbs are queued to the cancelled td list, many tds
> > might be located after hw dequeue pointer and just marked as no-op
> > but not reclaimed to num_trbs_free. This bias can leads to unnecessary
> > ring expansions and leaks in atomic pool.
>
> Good point, in that case trbs turned no-op never get added to free trb count.
>
> >
> > To prevent this bias, this patch counts free TRBs every time xhci moves
> > dequeue pointer. This patch utilizes existing
> > update_ring_for_set_deq_completion() function, renamed it to move_deq().
> >
> > When it walks through to the new dequeue pointer, it also counts
> > free TRBs manually. This patch adds a fast path for the most cases
> > where the new dequeue pointer is still in the current segment.
> >
>
> This looks like an option.
>
> Another approach would be to keep the normal case fast, and the special case code simple.
> Something like:
>
> finish_td()
> ...
>         /* Update ring dequeue pointer */
>         if (ep_ring->dequeue == td->first_trb) {
>                 ep_ring->dequeue = td->last_trb;
>                 ep_ring->deq_seg = td->last_trb_seg;
>                 ep_ring->num_trbs_free += td->num_trbs - 1;
>                 inc_deq(xhci, ep_ring);
>         } else {
>                 move_deq(...);
>         }
>
> move_deq(...)
> {
>         while(ring->dequeue != new_dequeue)
>                 inc_deq(ring);
>         inc_deq(ring);
> }

Yes, I think most cases would be in (ep_ring->dequeue == td->first_trb)
so I think just repeating inc_deq() will be okay like the above example
cancelling urbs is an expensive and unusual operation.

But as you can see, I changed update_ring_for_set_deq_completion() to
move_deq(),
Do you think it's okay for that substitution In xhci_handle_cmd_set_deq()?
I'm worrying about some weird situation where the new dequeue ptr is
not in the ring.

>
> inc_deq() increases the num_trbs_free count.
>
> I haven't looked at the details of this yet, but I'm away for the next two weeks so
> I wanted to share this first anyway.
>

Thanks for reviewing, I hope to get some feedback when you come back.

> -Mathias

On Fri, Jul 16, 2021 at 8:54 PM Mathias Nyman
<mathias.nyman@...ux.intel.com> wrote:
>
> On 8.7.2021 11.43, Ikjoon Jang wrote:
> > When unlinked urbs are queued to the cancelled td list, many tds
> > might be located after hw dequeue pointer and just marked as no-op
> > but not reclaimed to num_trbs_free. This bias can leads to unnecessary
> > ring expansions and leaks in atomic pool.
>
> Good point, in that case trbs turned no-op never get added to free trb count.
>
> >
> > To prevent this bias, this patch counts free TRBs every time xhci moves
> > dequeue pointer. This patch utilizes existing
> > update_ring_for_set_deq_completion() function, renamed it to move_deq().
> >
> > When it walks through to the new dequeue pointer, it also counts
> > free TRBs manually. This patch adds a fast path for the most cases
> > where the new dequeue pointer is still in the current segment.
> >
>
> This looks like an option.
>
> Another approach would be to keep the normal case fast, and the special case code simple.
> Something like:
>
> finish_td()
> ...
>         /* Update ring dequeue pointer */
>         if (ep_ring->dequeue == td->first_trb) {
>                 ep_ring->dequeue = td->last_trb;
>                 ep_ring->deq_seg = td->last_trb_seg;
>                 ep_ring->num_trbs_free += td->num_trbs - 1;
>                 inc_deq(xhci, ep_ring);
>         } else {
>                 move_deq(...);
>         }
>
> move_deq(...)
> {
>         while(ring->dequeue != new_dequeue)
>                 inc_deq(ring);
>         inc_deq(ring);
> }
>
> inc_deq() increases the num_trbs_free count.
>
> I haven't looked at the details of this yet, but I'm away for the next two weeks so
> I wanted to share this first anyway.
>
> -Mathias