netdev - Re: [PATCH net v3] skbuff: fix coalescing for page

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:   Fri, 1 Apr 2022 07:21:33 +0200
From:   Jesper Dangaard Brouer <jbrouer@...hat.com>
To:     Alexander Duyck <alexanderduyck@...com>,
        Jean-Philippe Brucker <jean-philippe@...aro.org>,
        "ilias.apalodimas@...aro.org" <ilias.apalodimas@...aro.org>,
        "linyunsheng@...wei.com" <linyunsheng@...wei.com>
Cc:     brouer@...hat.com, "hawk@...nel.org" <hawk@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net v3] skbuff: fix coalescing for page_pool fragment
 recycling



On 31/03/2022 20.44, Alexander Duyck wrote:
>> -----Original Message-----
>> From: Jean-Philippe Brucker <jean-philippe@...aro.org>
>> Sent: Thursday, March 31, 2022 3:25 AM
>> To: ilias.apalodimas@...aro.org; Alexander Duyck
>> <alexanderduyck@...com>; linyunsheng@...wei.com
>> Cc: hawk@...nel.org; davem@...emloft.net; kuba@...nel.org;
>> pabeni@...hat.com; netdev@...r.kernel.org; Jean-Philippe Brucker <jean-
>> philippe@...aro.org>
>> Subject: [PATCH net v3] skbuff: fix coalescing for page_pool fragment
>> recycling
>>
>> Fix a use-after-free when using page_pool with page fragments. We
>> encountered this problem during normal RX in the hns3 driver:
> 
> <snip>
> 
>> ---
>>   net/core/skbuff.c | 15 +++++++++++----
>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c index
>> ea51e23e9247..2d6ef6d7ebf5 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -5244,11 +5244,18 @@ bool skb_try_coalesce(struct sk_buff *to, struct
>> sk_buff *from,
>>   	if (skb_cloned(to))
>>   		return false;
>>
>> -	/* The page pool signature of struct page will eventually figure out
>> -	 * which pages can be recycled or not but for now let's prohibit slab
>> -	 * allocated and page_pool allocated SKBs from being coalesced.
>> +	/* In general, avoid mixing slab allocated and page_pool allocated
>> +	 * pages within the same SKB. However when @to is not pp_recycle
>> and
>> +	 * @from is cloned, we can transition frag pages from page_pool to
>> +	 * reference counted.
>> +	 *
>> +	 * On the other hand, don't allow coalescing two pp_recycle SKBs if
>> +	 * @from is cloned, in case the SKB is using page_pool fragment
>> +	 * references (PP_FLAG_PAGE_FRAG). Since we only take full page
>> +	 * references for cloned SKBs at the moment that would result in
>> +	 * inconsistent reference counts.
>>   	 */
>> -	if (to->pp_recycle != from->pp_recycle)
>> +	if (to->pp_recycle != (from->pp_recycle && !skb_cloned(from)))
>>   		return false;
>>
>>   	if (len <= skb_tailroom(to)) {
>> --
>> 2.25.1
> 
> This looks good to me. The impact should be minimal since it only applies to pp_recycle pages.
> 
> Reviewed-by: Alexander Duyck <alexanderduyck@...com>

I really appreciate Alex that you helped review this, thanks! :-)

Acked-by: Jesper Dangaard Brouer <brouer@...hat.com>