linux-kernel - Re: Pagecache zeroing: zero_user_segment, zero_user_segments and zero

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] [day] [month] [year] [list]

Date:	Wed, 16 May 2007 23:09:11 +0530
From:	"Satyam Sharma" <satyam.sharma@...il.com>
To:	"Andrew Morton" <akpm@...ux-foundation.org>
Cc:	"Christoph Lameter" <clameter@....com>,
	linux-kernel@...r.kernel.org,
	"Nate Diller" <nate.diller@...il.com>,
	"Nick Piggin" <nickpiggin@...oo.com.au>
Subject: Re: Pagecache zeroing: zero_user_segment, zero_user_segments and zero_user

On 5/16/07, Andrew Morton <akpm@...ux-foundation.org> wrote:
> >
> > 2. It has a useless parameter specifying the kmap slot.
> >    The functions above default to KM_USER0 which is also always used when
> >    zero_user_page was called except in one single case. We open code that
> >    single case to draw attention to the spot.
> >
>
> Dunno.  fwiw, we decided to _not_ embed KM_USER0 in the callee: we have had
> some pretty ghastly bugs in the past due to misuse of kmap slots so the
> idea was to shove the decision into the caller's face, make them think
> about what they're doing

Christoph hasn't really _replaced_ any !KM_USER0 case with
KM_USER0 in this patch. AFAIR, we had originally decided to not
embed KM_USER0 in zero_user_page because of this complaint
from Anton Altaparmakov:
http://lkml.org/lkml/2007/4/10/62

But Christoph's patch here seems to have left that fs/ntfs/aops.c case
alone, and used the zero_xxx helpers only for KM_USER0 cases, so
we should probably be safe.

> > +static inline void zero_user_segments(struct page *page,
> > +     unsigned start1, unsigned end1,
> > +     unsigned start2, unsigned end2)
> > +{
> > +     void *kaddr = kmap_atomic(page, KM_USER0);
> > +
> > +     BUG_ON(end1 > PAGE_SIZE ||
> > +             end2 > PAGE_SIZE);
> > +
> > +     if (end1 > start1)
> > +             memset(kaddr + start1, 0, end1 - start1);
> > +
> > +     if (end2 > start2)
> > +             memset(kaddr + start2, 0, end2 - start2);
> > +
> > +     flush_dcache_page(page);
> > +     kunmap_atomic(kaddr, KM_USER0);
> > +}
>
> For some reason we've always done the flush_dcache_page() while holding the
> kmap.  There's no reason for doing it this way and it just serves to worsen
> scheduling latency a tiny bit.

Wonder if any arch requires a valid mapping when doing the flush?

(reading Documentation/cachetlb.txt)

Documentation/cachetlb.txt is silent on whether flush_dcache_page()
really must be called before a kunmap(), but mentions that
flush_kernel_dcache_page() _must_ be called _before_ kunmap.
And I saw that parisc implements flush_dcache_page() using
flush_kernel_dcache_page itself (but then, we've not implemented a
meaningful kmap for parisc in the first place), so we've _just_
managed to be fine here ...

Still, flush_dcache_page() before kunmap'ing is an old practice,
would be wise to get this whetted by linux-arch first?

Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/