netdev - Re: [PATCH net-next 2/5] sfc: Use kmap_local_page() instead of kmap

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, 18 Nov 2022 12:34:03 -0800
From:   Anirudh Venkataramanan <anirudh.venkataramanan@...el.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>,
        <netdev@...r.kernel.org>
CC:     Ira Weiny <ira.weiny@...el.com>,
        Edward Cree <ecree.xilinx@...il.com>,
        Martin Habets <habetsm.xilinx@...il.com>
Subject: Re: [PATCH net-next 2/5] sfc: Use kmap_local_page() instead of
 kmap_atomic()

On 11/18/2022 11:26 AM, Fabio M. De Francesco wrote:
> On venerdì 18 novembre 2022 18:47:52 CET Anirudh Venkataramanan wrote:
>> On 11/18/2022 12:23 AM, Fabio M. De Francesco wrote:
>>> On giovedì 17 novembre 2022 23:25:54 CET Anirudh Venkataramanan wrote:
>>>> kmap_atomic() is being deprecated in favor of kmap_local_page().
>>>> Replace kmap_atomic() and kunmap_atomic() with kmap_local_page()
>>>> and kunmap_local() respectively.
>>>>
>>>> Note that kmap_atomic() disables preemption and page-fault processing,
> but
>>>> kmap_local_page() doesn't. Converting the former to the latter is safe
> only
>>>> if there isn't an implicit dependency on preemption and page-fault
> handling
>>>> being disabled, which does appear to be the case here.
>>>
>>> NIT: It is always possible to disable explicitly along with the
> conversion.
>>
>> Fair enough. I suppose "convert" is broader than "replace". How about this:
>>
>> "Replacing the former with the latter is safe only if there isn't an
>> implicit dependency on preemption and page-fault handling being
>> disabled, which does appear to be the case here."
>>
>> Ani
> 
> Let's start with 2/5 because it looks that here we are talking of a sensitive
> subject. Yesterday something triggered the necessity to make a patch for
> highmem.rst for clarifying that these conversions can _always_ be addressed.
> 
> I sent it to Ira and I'm waiting for his opinion before submitting it.
> 
> The me explain better... the point is that all kmap_atomic(), despite the
> differences, _can_ be converted to kmap_local_page().
> 
> What I care of is the safety of the conversions. I trust your commit message
> where you say that you inspected the code and that "there isn't an implicit
> dependency on preemption and page-fault handling being disabled".
> 
> I was talking about something very different: what if the code between mapping
> and unmapping was relying on implicit page-faults and/or preemption disable? I
> read between the lines that you consider a conversion of that kind something
> that cannot be addressed because "kmap_atomic() disables preemption and page-
> fault processing, but kmap_local_page() doesn't" (which is true).

No, I wasn't saying (or implying) this at all, nor did I think it 
could/would be interpreted this way.

I was trying to say that a straight-up replacing kmap_atomic() with 
kmap_local_page() would not be functionally safe if the code in between 
the mapping and unmapping relied on page-faults and/or preemption being 
disabled.

> 
> The point is that you have the possibility to convert also in this
> hypothetical case by doing something like the following.
> 
> Old code:
> 
> ptr = kmap_atomic(page);
> do something with ptr;
> kunmap_atomic(ptr);
> 
> You checked the code and understood that that "something" can only be carried
> out with page-faults disabled (just an example). Conversion:
> 
> pagefault_disable();
> ptr = kmap_local_page(page);
> do something with ptr;
> kunmap_local(ptr);
> pagefault_enable();
> 
> I'm not asking to reword your commit message only for the purpose to clear
> that your changes are "safe" because you checked the code and can reasonably
> affirm that the conversion doesn't depend on further disables.
> 
> I just said it to make you notice that every kmap_atomic() conversion to
> kmap_local_page() is "safe", but only if you really understand the code and
> act accordingly.
> 
> I'm too wordy, Ira said it so many times. Unfortunately, I'm not able to
> optimize English text and need to improve. I'm sorry.
> 
> Does my long explanation make any sense to you?
> 
> If so, I'm happy. I'm not asking to send v2. I just desired that you realize
> (1) how tricky these conversions may be and therefore how much important is
> not to do them mechanically (2) how to better craft your next commit message
> (if you want to keep on helping with these conversions).

Appreciate the explanation, but as I explained above, what you read 
between the lines isn't what I was saying. I am most certainly not 
making these changes mechanically.

> 
> I'm OK with this patch. Did you see my tag? :-)

I did, and thanks!

FYI, I will be sending a v2 for the series anyway (with the 
memcpy_from_page()) changes, and so I am planning to update the commit 
messages to hopefully be a little clearer to you.

Ani