netdev - Re: [PATCH] ionic: fix 16bit math issue when 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]

Message-ID: <331b81ab-7a19-0055-088f-d2f595c26303@linux.vnet.ibm.com>
Date: Tue, 12 Sep 2023 13:48:15 -0700
From: David Christensen <drc@...ux.vnet.ibm.com>
To: Jacob Keller <jacob.e.keller@...el.com>, shannon.nelson@....com,
        brett.creeley@....com, drivers@...sando.io
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH] ionic: fix 16bit math issue when PAGE_SIZE >= 64KB



On 9/11/23 5:14 PM, Jacob Keller wrote:
> 
> 
> On 9/11/2023 3:22 PM, David Christensen wrote:
>> The function ionic_rx_fill() uses 16bit math when calculating the
>> the number of pages required for an RX descriptor given an interface
>> MTU setting. If the system PAGE_SIZE >= 64KB, the frag_len and
>> remain_len values will always be 0, causing unnecessary scatter-
>> gather elements to be assigned to the RX descriptor, up to the
>> maximum number of scatter-gather elements per descriptor.
>>
>> A similar change in ionic_rx_frags() is implemented for symmetry,
>> but has not been observed as an issue since scatter-gather
>> elements are not necessary for such larger page sizes.
>>
>> Fixes: 4b0a7539a372 ("ionic: implement Rx page reuse")
>> Signed-off-by: David Christensen <drc@...ux.vnet.ibm.com>
>> ---
> 
> Given this is a bug fix, it should probably have a subject of [PATCH
> net] or [net] to indicate its targeting the net tree.

Will resend v2 with updated Subject line.

> 
> I'm not sure I follow the logic for frag_len and remain_len always being
> zero, since typecasting unsigned values truncates the higher bytes
> (technically its guaranteed by the standard to result in the smallest
> value congruent modulo 2^16 for a 16bit typecast), so if page_offset was
> non-zero then the resulting with the typecast should be as well.. but
> either way its definitely not going to work as desired.

Sorry, tried condensing the explanation too much. I'm not sure how 
frequently buf_info->page_offset is non-zero, but when 
ionic_rx_page_alloc() allocates a new page, as happens during initial 
driver load, it explicitly sets buf_info->page_offset to 0.  As a 
result, the remain_len value never decreases from the original frame 
size (e.g. 1522) while frag_len is always calculated as 0 ((min_t(u16, 
0x5F2, (0x1_0000 - 0) -> 0).

I'll update the the description in v2.

Dave