netdev - Re: [PATCH net] tcp: add sanity checks to rx zerocopy

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: <CAOFY-A3pi-FNbe_=ED+4HGimBdLq95xiom++Jd6f9aRrbEssBA@mail.gmail.com>
Date: Thu, 25 Jan 2024 14:23:43 -0800
From: Arjun Roy <arjunroy@...gle.com>
To: Matthew Wilcox <willy@...radead.org>
Cc: Eric Dumazet <edumazet@...gle.com>, "David S . Miller" <davem@...emloft.net>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Neal Cardwell <ncardwell@...gle.com>, netdev@...r.kernel.org, eric.dumazet@...il.com, 
	ZhangPeng <zhangpeng362@...wei.com>, linux-mm@...r.kernel.org, 
	Andrew Morton <akpm@...ux-foundation.org>, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH net] tcp: add sanity checks to rx zerocopy

On Thu, Jan 25, 2024 at 8:07 AM Matthew Wilcox <willy@...radead.org> wrote:
>
> On Thu, Jan 25, 2024 at 10:33:17AM +0000, Eric Dumazet wrote:
> > +++ b/net/ipv4/tcp.c
> > @@ -1786,7 +1786,17 @@ static skb_frag_t *skb_advance_to_frag(struct sk_buff *skb, u32 offset_skb,
> >
> >  static bool can_map_frag(const skb_frag_t *frag)
> >  {
> > -     return skb_frag_size(frag) == PAGE_SIZE && !skb_frag_off(frag);
> > +     struct page *page;
> > +
> > +     if (skb_frag_size(frag) != PAGE_SIZE || skb_frag_off(frag))
> > +             return false;
> > +
> > +     page = skb_frag_page(frag);
> > +
> > +     if (PageCompound(page) || page->mapping)
> > +             return false;
>
> I'm not entirely sure why you're testing PageCompound here.  If a driver
> allocates a compound page, we'd still want to be able to insert it,
> right?
>

Resend b/c I forgot I was in HTML mode email, oops.

Is there a common use case for a NIC driver to be doing this? I was
under the impression NIC drivers would get pages individually since it
would be harder to find physically contiguous groupings in general.

Anyways, a possible reason to not allow compound pages - if we have
some large memory range and different receiving users are interested
in different parts of the range in userspace - since the whole range
is pinned till everyone is done with it, it can lead to worse cases of
memory lingering when some user only wanted 10 bytes out of some N *
PAGE_SIZE range for a multi-minute long period.

-Arjun


> I have a feeling that we want to fix this in the VM layer.  There are
> some weird places calling vm_insert_page() and we should probably make
> them all fail.
>
> Something like this, perhaps?
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 1a60faad2e49..ae0abab56d38 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1871,6 +1871,10 @@ static int insert_page_into_pte_locked(struct vm_area_struct *vma, pte_t *pte,
>
>         if (!pte_none(ptep_get(pte)))
>                 return -EBUSY;
> +       if (folio->mapping &&
> +           ((addr - vma->vm_start) / PAGE_SIZE + vma->vm_pgoff) !=
> +           (folio->index + folio_page_idx(folio, page)))
> +               return -EINVAL;
>         /* Ok, finally just insert the thing.. */
>         folio_get(folio);
>         inc_mm_counter(vma->vm_mm, mm_counter_file(folio));