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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <e13a1d38-cc0a-51b4-fe19-7a9145e75b1d@linux.intel.com>
Date:   Fri, 16 Jul 2021 15:56:35 +0300
From:   Mathias Nyman <mathias.nyman@...ux.intel.com>
To:     Ikjoon Jang <ikjn@...omium.org>, linux-usb@...r.kernel.org
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Mathias Nyman <mathias.nyman@...el.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] xhci: fix unmatched num_trbs_free

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