netdev - Re: [PATCH net v3] net: allow skb_datagram_iter to be called from any context

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CANn89iLA-0Vo=9b4SSJP=9BhnLOTKz2khdq6TG+tJpey8DpKCg@mail.gmail.com>
Date: Wed, 26 Jun 2024 09:40:43 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Sagi Grimberg <sagi@...mberg.me>
Cc: netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>, 
	David Howells <dhowells@...hat.com>, Matthew Wilcox <willy@...radead.org>
Subject: Re: [PATCH net v3] net: allow skb_datagram_iter to be called from any context

On Wed, Jun 26, 2024 at 9:00 AM Sagi Grimberg <sagi@...mberg.me> wrote:
>
> We only use the mapping in a single context, so kmap_local is sufficient
> and cheaper. Make sure to use skb_frag_foreach_page as skb frags may
> contain highmem compound pages and we need to map page by page.
>
> Reported-by: kernel test robot <oliver.sang@...el.com>
> Closes: https://lore.kernel.org/oe-lkp/202406161539.b5ff7b20-oliver.sang@intel.com
> Signed-off-by: Sagi Grimberg <sagi@...mberg.me>

Thanks for working on this !

A patch targeting net tree would need a Fixes: tag, so that stable
teams do not have
to do archeological digging to find which trees need this fix.

If the bug is too old, then maybe this patch should use kmap() instead
of  kmap_local_page() ?

Then in net-next, (after this fix is merged), perform the conversion
to kmap_local_page()

Fact that the bug never showed up is a bit strange, are 32bit systems
still used today ? (apart from bots)...

Do we have a reproducer to test this?

> ---
> Changes from v2:
> - added a target tree in subject prefix
> - added reported credits and closes annotation
>
> Changes from v1:
> - Fix usercopy BUG() due to copy from highmem pages across page boundary
>   by using skb_frag_foreach_page
>
>  net/core/datagram.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/net/core/datagram.c b/net/core/datagram.c
> index e614cfd8e14a..e9ba4c7b449d 100644
> --- a/net/core/datagram.c
> +++ b/net/core/datagram.c
> @@ -416,15 +416,22 @@ static int __skb_datagram_iter(const struct sk_buff *skb, int offset,
>
>                 end = start + skb_frag_size(frag);
>                 if ((copy = end - offset) > 0) {
> -                       struct page *page = skb_frag_page(frag);
> -                       u8 *vaddr = kmap(page);
> +                       u32 p_off, p_len, copied;
> +                       struct page *p;
> +                       u8 *vaddr;
>
>                         if (copy > len)
>                                 copy = len;
> -                       n = INDIRECT_CALL_1(cb, simple_copy_to_iter,
> -                                       vaddr + skb_frag_off(frag) + offset - start,
> -                                       copy, data, to);
> -                       kunmap(page);
> +
> +                       skb_frag_foreach_page(frag,
> +                                             skb_frag_off(frag) + offset - start,
> +                                             copy, p, p_off, p_len, copied) {
> +                               vaddr = kmap_local_page(p);
> +                               n = INDIRECT_CALL_1(cb, simple_copy_to_iter,
> +                                       vaddr + p_off, p_len, data, to);
> +                               kunmap_local(vaddr);
> +                       }
> +
>                         offset += n;
>                         if (n != copy)
>                                 goto short_copy;
> --
> 2.43.0
>