netdev - Re: [PATCH v2 net-next 4/6] cassini: Use memcpy_from_page() instead of k[un]map

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, 25 Nov 2022 16:39:21 +0100
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     netdev@...r.kernel.org,
        Anirudh Venkataramanan <anirudh.venkataramanan@...el.com>
Cc:     Ira Weiny <ira.weiny@...el.com>,
        Anirudh Venkataramanan <anirudh.venkataramanan@...el.com>
Subject: Re: [PATCH v2 net-next 4/6] cassini: Use memcpy_from_page() instead of k[un]map_atomic()

On mercoledì 23 novembre 2022 21:52:17 CET Anirudh Venkataramanan wrote:
> kmap_atomic() is being deprecated in favor of kmap_local_page(). Replace
> the map-memcpy-unmap usage pattern (done using k[un]map_atomic()) with
> memcpy_from_page(), which internally uses kmap_local_page() and
> kunmap_local(). This renders the variable 'vaddr' unnecessary, and so
> remove this too.
> 
> Note that kmap_atomic() disables preemption and page-fault processing, but
> kmap_local_page() doesn't. When converting uses of kmap_atomic(), one has
> to check if the code being executed between the map/unmap implicitly
> depends on page-faults and/or preemption being disabled. If yes, then code
> to disable page-faults and/or preemption should also be added for
> functional correctness. That however doesn't appear to be the case here,
> so just memcpy_from_page() is used.
> 
> Also note that the page being mapped is not allocated by the driver, and so
> the driver doesn't know if the page is in normal memory. This is the reason
> kmap_local_page() is used (via memcpy_from_page()) as opposed to
> page_address().
> 
> I don't have hardware, so this change has only been compile tested.
> 
> Cc: Ira Weiny <ira.weiny@...el.com>
> Cc: Fabio M. De Francesco <fmdefrancesco@...il.com>
> Suggested-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
> Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@...el.com>
> ---
> v1 -> v2:
>  Use memcpy_from_page() as suggested by Fabio
>  Add a "Suggested-by" tag
>  Rework commit message
> ---
>  drivers/net/ethernet/sun/cassini.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)

Reviewed-by: Fabio M. De Francesco <fmdefrancesco@...il.com>

Thanks,

Fabio

> diff --git a/drivers/net/ethernet/sun/cassini.c
> b/drivers/net/ethernet/sun/cassini.c index 2f66cfc..4ef05ba 100644
> --- a/drivers/net/ethernet/sun/cassini.c
> +++ b/drivers/net/ethernet/sun/cassini.c
> @@ -90,8 +90,6 @@
>  #include <linux/uaccess.h>
>  #include <linux/jiffies.h>
> 
> -#define cas_page_map(x)      kmap_atomic((x))
> -#define cas_page_unmap(x)    kunmap_atomic((x))
>  #define CAS_NCPUS            num_online_cpus()
> 
>  #define cas_skb_release(x)  netif_rx(x)
> @@ -2781,18 +2779,14 @@ static inline int cas_xmit_tx_ringN(struct cas *cp,
> int ring,
> 
>  		tabort = cas_calc_tabort(cp, skb_frag_off(fragp), len);
>  		if (unlikely(tabort)) {
> -			void *addr;
> -
>  			/* NOTE: len is always > tabort */
>  			cas_write_txd(cp, ring, entry, mapping, len - 
tabort,
>  				      ctrl, 0);
>  			entry = TX_DESC_NEXT(ring, entry);
> -
> -			addr = cas_page_map(skb_frag_page(fragp));
> -			memcpy(tx_tiny_buf(cp, ring, entry),
> -			       addr + skb_frag_off(fragp) + len - 
tabort,
> -			       tabort);
> -			cas_page_unmap(addr);
> +			memcpy_from_page(tx_tiny_buf(cp, ring, entry),
> +					 skb_frag_page(fragp),
> +					 skb_frag_off(fragp) + len - 
tabort,
> +					 tabort);
>  			mapping = tx_tiny_map(cp, ring, entry, tentry);
>  			len     = tabort;
>  		}
> --
> 2.37.2