* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-07 22:57 ` [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core ira.weiny
@ 2020-12-07 23:26 ` Matthew Wilcox
2020-12-07 23:34 ` Dan Williams
2020-12-08 0:40 ` kernel test robot
` (2 subsequent siblings)
3 siblings, 1 reply; 30+ messages in thread
From: Matthew Wilcox @ 2020-12-07 23:26 UTC (permalink / raw)
To: ira.weiny
Cc: Thomas Gleixner, Andrew Morton, Dave Hansen, Christoph Hellwig,
Dan Williams, Al Viro, Eric Biggers, Joonas Lahtinen,
linux-kernel, linux-fsdevel
On Mon, Dec 07, 2020 at 02:57:03PM -0800, ira.weiny@intel.com wrote:
> +static inline void memcpy_page(struct page *dst_page, size_t dst_off,
> + struct page *src_page, size_t src_off,
> + size_t len)
> +{
> + char *dst = kmap_local_page(dst_page);
> + char *src = kmap_local_page(src_page);
I appreciate you've only moved these, but please add:
BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
> + memcpy(dst + dst_off, src + src_off, len);
> + kunmap_local(src);
> + kunmap_local(dst);
> +}
> +
> +static inline void memmove_page(struct page *dst_page, size_t dst_off,
> + struct page *src_page, size_t src_off,
> + size_t len)
> +{
> + char *dst = kmap_local_page(dst_page);
> + char *src = kmap_local_page(src_page);
BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
> + memmove(dst + dst_off, src + src_off, len);
> + kunmap_local(src);
> + kunmap_local(dst);
> +}
> +
> +static inline void memcpy_from_page(char *to, struct page *page, size_t offset, size_t len)
> +{
> + char *from = kmap_local_page(page);
BUG_ON(offset + len > PAGE_SIZE);
> + memcpy(to, from + offset, len);
> + kunmap_local(from);
> +}
> +
> +static inline void memcpy_to_page(struct page *page, size_t offset, const char *from, size_t len)
> +{
> + char *to = kmap_local_page(page);
BUG_ON(offset + len > PAGE_SIZE);
> + memcpy(to + offset, from, len);
> + kunmap_local(to);
> +}
> +
> +static inline void memset_page(struct page *page, size_t offset, int val, size_t len)
> +{
> + char *addr = kmap_local_page(page);
BUG_ON(offset + len > PAGE_SIZE);
> + memset(addr + offset, val, len);
> + kunmap_local(addr);
> +}
> +
> #endif /* _LINUX_PAGEMAP_H */
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-07 23:26 ` Matthew Wilcox
@ 2020-12-07 23:34 ` Dan Williams
2020-12-07 23:40 ` Matthew Wilcox
0 siblings, 1 reply; 30+ messages in thread
From: Dan Williams @ 2020-12-07 23:34 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Weiny, Ira, Thomas Gleixner, Andrew Morton, Dave Hansen,
Christoph Hellwig, Al Viro, Eric Biggers, Joonas Lahtinen,
Linux Kernel Mailing List, linux-fsdevel
On Mon, Dec 7, 2020 at 3:27 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Dec 07, 2020 at 02:57:03PM -0800, ira.weiny@intel.com wrote:
> > +static inline void memcpy_page(struct page *dst_page, size_t dst_off,
> > + struct page *src_page, size_t src_off,
> > + size_t len)
> > +{
> > + char *dst = kmap_local_page(dst_page);
> > + char *src = kmap_local_page(src_page);
>
> I appreciate you've only moved these, but please add:
>
> BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
I imagine it's not outside the realm of possibility that some driver
on CONFIG_HIGHMEM=n is violating this assumption and getting away with
it because kmap_atomic() of contiguous pages "just works (TM)".
Shouldn't this WARN rather than BUG so that the user can report the
buggy driver and not have a dead system?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-07 23:34 ` Dan Williams
@ 2020-12-07 23:40 ` Matthew Wilcox
2020-12-07 23:49 ` Dan Williams
0 siblings, 1 reply; 30+ messages in thread
From: Matthew Wilcox @ 2020-12-07 23:40 UTC (permalink / raw)
To: Dan Williams
Cc: Weiny, Ira, Thomas Gleixner, Andrew Morton, Dave Hansen,
Christoph Hellwig, Al Viro, Eric Biggers, Joonas Lahtinen,
Linux Kernel Mailing List, linux-fsdevel
On Mon, Dec 07, 2020 at 03:34:44PM -0800, Dan Williams wrote:
> On Mon, Dec 7, 2020 at 3:27 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Mon, Dec 07, 2020 at 02:57:03PM -0800, ira.weiny@intel.com wrote:
> > > +static inline void memcpy_page(struct page *dst_page, size_t dst_off,
> > > + struct page *src_page, size_t src_off,
> > > + size_t len)
> > > +{
> > > + char *dst = kmap_local_page(dst_page);
> > > + char *src = kmap_local_page(src_page);
> >
> > I appreciate you've only moved these, but please add:
> >
> > BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
>
> I imagine it's not outside the realm of possibility that some driver
> on CONFIG_HIGHMEM=n is violating this assumption and getting away with
> it because kmap_atomic() of contiguous pages "just works (TM)".
> Shouldn't this WARN rather than BUG so that the user can report the
> buggy driver and not have a dead system?
As opposed to (on a HIGHMEM=y system) silently corrupting data that
is on the next page of memory? I suppose ideally ...
if (WARN_ON(dst_off + len > PAGE_SIZE))
len = PAGE_SIZE - dst_off;
if (WARN_ON(src_off + len > PAGE_SIZE))
len = PAGE_SIZE - src_off;
and then we just truncate the data of the offending caller instead of
corrupting innocent data that happens to be adjacent. Although that's
not ideal either ... I dunno, what's the least bad poison to drink here?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-07 23:40 ` Matthew Wilcox
@ 2020-12-07 23:49 ` Dan Williams
2020-12-08 21:32 ` Ira Weiny
0 siblings, 1 reply; 30+ messages in thread
From: Dan Williams @ 2020-12-07 23:49 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Weiny, Ira, Thomas Gleixner, Andrew Morton, Dave Hansen,
Christoph Hellwig, Al Viro, Eric Biggers, Joonas Lahtinen,
Linux Kernel Mailing List, linux-fsdevel
On Mon, Dec 7, 2020 at 3:40 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Dec 07, 2020 at 03:34:44PM -0800, Dan Williams wrote:
> > On Mon, Dec 7, 2020 at 3:27 PM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Mon, Dec 07, 2020 at 02:57:03PM -0800, ira.weiny@intel.com wrote:
> > > > +static inline void memcpy_page(struct page *dst_page, size_t dst_off,
> > > > + struct page *src_page, size_t src_off,
> > > > + size_t len)
> > > > +{
> > > > + char *dst = kmap_local_page(dst_page);
> > > > + char *src = kmap_local_page(src_page);
> > >
> > > I appreciate you've only moved these, but please add:
> > >
> > > BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
> >
> > I imagine it's not outside the realm of possibility that some driver
> > on CONFIG_HIGHMEM=n is violating this assumption and getting away with
> > it because kmap_atomic() of contiguous pages "just works (TM)".
> > Shouldn't this WARN rather than BUG so that the user can report the
> > buggy driver and not have a dead system?
>
> As opposed to (on a HIGHMEM=y system) silently corrupting data that
> is on the next page of memory?
Wouldn't it fault in HIGHMEM=y case? I guess not necessarily...
> I suppose ideally ...
>
> if (WARN_ON(dst_off + len > PAGE_SIZE))
> len = PAGE_SIZE - dst_off;
> if (WARN_ON(src_off + len > PAGE_SIZE))
> len = PAGE_SIZE - src_off;
>
> and then we just truncate the data of the offending caller instead of
> corrupting innocent data that happens to be adjacent. Although that's
> not ideal either ... I dunno, what's the least bad poison to drink here?
Right, if the driver was relying on "corruption" for correct operation.
If corruption actual were happening in practice wouldn't there have
been screams by now? Again, not necessarily...
At least with just plain WARN the kernel will start screaming on the
user's behalf, and if it worked before it will keep working.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-07 23:49 ` Dan Williams
@ 2020-12-08 21:32 ` Ira Weiny
2020-12-08 21:50 ` Matthew Wilcox
2020-12-08 22:21 ` Dan Williams
0 siblings, 2 replies; 30+ messages in thread
From: Ira Weiny @ 2020-12-08 21:32 UTC (permalink / raw)
To: Dan Williams
Cc: Matthew Wilcox, Thomas Gleixner, Andrew Morton, Dave Hansen,
Christoph Hellwig, Al Viro, Eric Biggers, Joonas Lahtinen,
Linux Kernel Mailing List, linux-fsdevel
On Mon, Dec 07, 2020 at 03:49:55PM -0800, Dan Williams wrote:
> On Mon, Dec 7, 2020 at 3:40 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Mon, Dec 07, 2020 at 03:34:44PM -0800, Dan Williams wrote:
> > > On Mon, Dec 7, 2020 at 3:27 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > >
> > > > On Mon, Dec 07, 2020 at 02:57:03PM -0800, ira.weiny@intel.com wrote:
> > > > > +static inline void memcpy_page(struct page *dst_page, size_t dst_off,
> > > > > + struct page *src_page, size_t src_off,
> > > > > + size_t len)
> > > > > +{
> > > > > + char *dst = kmap_local_page(dst_page);
> > > > > + char *src = kmap_local_page(src_page);
> > > >
> > > > I appreciate you've only moved these, but please add:
> > > >
> > > > BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
> > >
> > > I imagine it's not outside the realm of possibility that some driver
> > > on CONFIG_HIGHMEM=n is violating this assumption and getting away with
> > > it because kmap_atomic() of contiguous pages "just works (TM)".
> > > Shouldn't this WARN rather than BUG so that the user can report the
> > > buggy driver and not have a dead system?
> >
> > As opposed to (on a HIGHMEM=y system) silently corrupting data that
> > is on the next page of memory?
>
> Wouldn't it fault in HIGHMEM=y case? I guess not necessarily...
>
> > I suppose ideally ...
> >
> > if (WARN_ON(dst_off + len > PAGE_SIZE))
> > len = PAGE_SIZE - dst_off;
> > if (WARN_ON(src_off + len > PAGE_SIZE))
> > len = PAGE_SIZE - src_off;
> >
> > and then we just truncate the data of the offending caller instead of
> > corrupting innocent data that happens to be adjacent. Although that's
> > not ideal either ... I dunno, what's the least bad poison to drink here?
>
> Right, if the driver was relying on "corruption" for correct operation.
>
> If corruption actual were happening in practice wouldn't there have
> been screams by now? Again, not necessarily...
>
> At least with just plain WARN the kernel will start screaming on the
> user's behalf, and if it worked before it will keep working.
So I decided to just sleep on this because I was recently told to not introduce
new WARN_ON's[1]
I don't think that truncating len is worth the effort. The conversions being
done should all 'work' At least corrupting users data in the same way as it
used to... ;-) I'm ok with adding the WARN_ON's and I have modified the patch
to do so while I work through the 0-day issues. (not sure what is going on
there.)
However, are we ok with adding the WARN_ON's given what Greg KH told me? This
is a bit more critical than the PKS API in that it could result in corrupt
data.
Ira
[1] https://lore.kernel.org/linux-doc/20201103065024.GC75930@kroah.com/
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-08 21:32 ` Ira Weiny
@ 2020-12-08 21:50 ` Matthew Wilcox
2020-12-08 22:23 ` Dan Williams
2020-12-08 22:21 ` Dan Williams
1 sibling, 1 reply; 30+ messages in thread
From: Matthew Wilcox @ 2020-12-08 21:50 UTC (permalink / raw)
To: Ira Weiny
Cc: Dan Williams, Thomas Gleixner, Andrew Morton, Dave Hansen,
Christoph Hellwig, Al Viro, Eric Biggers, Joonas Lahtinen,
Linux Kernel Mailing List, linux-fsdevel
On Tue, Dec 08, 2020 at 01:32:55PM -0800, Ira Weiny wrote:
> On Mon, Dec 07, 2020 at 03:49:55PM -0800, Dan Williams wrote:
> > On Mon, Dec 7, 2020 at 3:40 PM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Mon, Dec 07, 2020 at 03:34:44PM -0800, Dan Williams wrote:
> > > > On Mon, Dec 7, 2020 at 3:27 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > > >
> > > > > On Mon, Dec 07, 2020 at 02:57:03PM -0800, ira.weiny@intel.com wrote:
> > > > > > +static inline void memcpy_page(struct page *dst_page, size_t dst_off,
> > > > > > + struct page *src_page, size_t src_off,
> > > > > > + size_t len)
> > > > > > +{
> > > > > > + char *dst = kmap_local_page(dst_page);
> > > > > > + char *src = kmap_local_page(src_page);
> > > > >
> > > > > I appreciate you've only moved these, but please add:
> > > > >
> > > > > BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
> > > >
> > > > I imagine it's not outside the realm of possibility that some driver
> > > > on CONFIG_HIGHMEM=n is violating this assumption and getting away with
> > > > it because kmap_atomic() of contiguous pages "just works (TM)".
> > > > Shouldn't this WARN rather than BUG so that the user can report the
> > > > buggy driver and not have a dead system?
> > >
> > > As opposed to (on a HIGHMEM=y system) silently corrupting data that
> > > is on the next page of memory?
> >
> > Wouldn't it fault in HIGHMEM=y case? I guess not necessarily...
> >
> > > I suppose ideally ...
> > >
> > > if (WARN_ON(dst_off + len > PAGE_SIZE))
> > > len = PAGE_SIZE - dst_off;
> > > if (WARN_ON(src_off + len > PAGE_SIZE))
> > > len = PAGE_SIZE - src_off;
> > >
> > > and then we just truncate the data of the offending caller instead of
> > > corrupting innocent data that happens to be adjacent. Although that's
> > > not ideal either ... I dunno, what's the least bad poison to drink here?
> >
> > Right, if the driver was relying on "corruption" for correct operation.
> >
> > If corruption actual were happening in practice wouldn't there have
> > been screams by now? Again, not necessarily...
> >
> > At least with just plain WARN the kernel will start screaming on the
> > user's behalf, and if it worked before it will keep working.
>
> So I decided to just sleep on this because I was recently told to not introduce
> new WARN_ON's[1]
>
> I don't think that truncating len is worth the effort. The conversions being
> done should all 'work' At least corrupting users data in the same way as it
> used to... ;-) I'm ok with adding the WARN_ON's and I have modified the patch
> to do so while I work through the 0-day issues. (not sure what is going on
> there.)
>
> However, are we ok with adding the WARN_ON's given what Greg KH told me? This
> is a bit more critical than the PKS API in that it could result in corrupt
> data.
zero_user_segments contains:
BUG_ON(end1 > page_size(page) || end2 > page_size(page));
These should be consistent. I think we've demonstrated that there is
no good option here.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-08 21:50 ` Matthew Wilcox
@ 2020-12-08 22:23 ` Dan Williams
2020-12-08 22:32 ` Matthew Wilcox
0 siblings, 1 reply; 30+ messages in thread
From: Dan Williams @ 2020-12-08 22:23 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Ira Weiny, Thomas Gleixner, Andrew Morton, Dave Hansen,
Christoph Hellwig, Al Viro, Eric Biggers, Joonas Lahtinen,
Linux Kernel Mailing List, linux-fsdevel
On Tue, Dec 8, 2020 at 1:51 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Tue, Dec 08, 2020 at 01:32:55PM -0800, Ira Weiny wrote:
> > On Mon, Dec 07, 2020 at 03:49:55PM -0800, Dan Williams wrote:
> > > On Mon, Dec 7, 2020 at 3:40 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > >
> > > > On Mon, Dec 07, 2020 at 03:34:44PM -0800, Dan Williams wrote:
> > > > > On Mon, Dec 7, 2020 at 3:27 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > > > >
> > > > > > On Mon, Dec 07, 2020 at 02:57:03PM -0800, ira.weiny@intel.com wrote:
> > > > > > > +static inline void memcpy_page(struct page *dst_page, size_t dst_off,
> > > > > > > + struct page *src_page, size_t src_off,
> > > > > > > + size_t len)
> > > > > > > +{
> > > > > > > + char *dst = kmap_local_page(dst_page);
> > > > > > > + char *src = kmap_local_page(src_page);
> > > > > >
> > > > > > I appreciate you've only moved these, but please add:
> > > > > >
> > > > > > BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
> > > > >
> > > > > I imagine it's not outside the realm of possibility that some driver
> > > > > on CONFIG_HIGHMEM=n is violating this assumption and getting away with
> > > > > it because kmap_atomic() of contiguous pages "just works (TM)".
> > > > > Shouldn't this WARN rather than BUG so that the user can report the
> > > > > buggy driver and not have a dead system?
> > > >
> > > > As opposed to (on a HIGHMEM=y system) silently corrupting data that
> > > > is on the next page of memory?
> > >
> > > Wouldn't it fault in HIGHMEM=y case? I guess not necessarily...
> > >
> > > > I suppose ideally ...
> > > >
> > > > if (WARN_ON(dst_off + len > PAGE_SIZE))
> > > > len = PAGE_SIZE - dst_off;
> > > > if (WARN_ON(src_off + len > PAGE_SIZE))
> > > > len = PAGE_SIZE - src_off;
> > > >
> > > > and then we just truncate the data of the offending caller instead of
> > > > corrupting innocent data that happens to be adjacent. Although that's
> > > > not ideal either ... I dunno, what's the least bad poison to drink here?
> > >
> > > Right, if the driver was relying on "corruption" for correct operation.
> > >
> > > If corruption actual were happening in practice wouldn't there have
> > > been screams by now? Again, not necessarily...
> > >
> > > At least with just plain WARN the kernel will start screaming on the
> > > user's behalf, and if it worked before it will keep working.
> >
> > So I decided to just sleep on this because I was recently told to not introduce
> > new WARN_ON's[1]
> >
> > I don't think that truncating len is worth the effort. The conversions being
> > done should all 'work' At least corrupting users data in the same way as it
> > used to... ;-) I'm ok with adding the WARN_ON's and I have modified the patch
> > to do so while I work through the 0-day issues. (not sure what is going on
> > there.)
> >
> > However, are we ok with adding the WARN_ON's given what Greg KH told me? This
> > is a bit more critical than the PKS API in that it could result in corrupt
> > data.
>
> zero_user_segments contains:
>
> BUG_ON(end1 > page_size(page) || end2 > page_size(page));
>
> These should be consistent. I think we've demonstrated that there is
> no good option here.
True, but these helpers are being deployed to many new locations where
they were not used before.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-08 22:23 ` Dan Williams
@ 2020-12-08 22:32 ` Matthew Wilcox
2020-12-08 22:45 ` Darrick J. Wong
0 siblings, 1 reply; 30+ messages in thread
From: Matthew Wilcox @ 2020-12-08 22:32 UTC (permalink / raw)
To: Dan Williams
Cc: Ira Weiny, Thomas Gleixner, Andrew Morton, Dave Hansen,
Christoph Hellwig, Al Viro, Eric Biggers, Joonas Lahtinen,
Linux Kernel Mailing List, linux-fsdevel
On Tue, Dec 08, 2020 at 02:23:10PM -0800, Dan Williams wrote:
> On Tue, Dec 8, 2020 at 1:51 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Tue, Dec 08, 2020 at 01:32:55PM -0800, Ira Weiny wrote:
> > > On Mon, Dec 07, 2020 at 03:49:55PM -0800, Dan Williams wrote:
> > > > On Mon, Dec 7, 2020 at 3:40 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > > >
> > > > > On Mon, Dec 07, 2020 at 03:34:44PM -0800, Dan Williams wrote:
> > > > > > On Mon, Dec 7, 2020 at 3:27 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > > > > >
> > > > > > > On Mon, Dec 07, 2020 at 02:57:03PM -0800, ira.weiny@intel.com wrote:
> > > > > > > > +static inline void memcpy_page(struct page *dst_page, size_t dst_off,
> > > > > > > > + struct page *src_page, size_t src_off,
> > > > > > > > + size_t len)
> > > > > > > > +{
> > > > > > > > + char *dst = kmap_local_page(dst_page);
> > > > > > > > + char *src = kmap_local_page(src_page);
> > > > > > >
> > > > > > > I appreciate you've only moved these, but please add:
> > > > > > >
> > > > > > > BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
> > > > > >
> > > > > > I imagine it's not outside the realm of possibility that some driver
> > > > > > on CONFIG_HIGHMEM=n is violating this assumption and getting away with
> > > > > > it because kmap_atomic() of contiguous pages "just works (TM)".
> > > > > > Shouldn't this WARN rather than BUG so that the user can report the
> > > > > > buggy driver and not have a dead system?
> > > > >
> > > > > As opposed to (on a HIGHMEM=y system) silently corrupting data that
> > > > > is on the next page of memory?
> > > >
> > > > Wouldn't it fault in HIGHMEM=y case? I guess not necessarily...
> > > >
> > > > > I suppose ideally ...
> > > > >
> > > > > if (WARN_ON(dst_off + len > PAGE_SIZE))
> > > > > len = PAGE_SIZE - dst_off;
> > > > > if (WARN_ON(src_off + len > PAGE_SIZE))
> > > > > len = PAGE_SIZE - src_off;
> > > > >
> > > > > and then we just truncate the data of the offending caller instead of
> > > > > corrupting innocent data that happens to be adjacent. Although that's
> > > > > not ideal either ... I dunno, what's the least bad poison to drink here?
> > > >
> > > > Right, if the driver was relying on "corruption" for correct operation.
> > > >
> > > > If corruption actual were happening in practice wouldn't there have
> > > > been screams by now? Again, not necessarily...
> > > >
> > > > At least with just plain WARN the kernel will start screaming on the
> > > > user's behalf, and if it worked before it will keep working.
> > >
> > > So I decided to just sleep on this because I was recently told to not introduce
> > > new WARN_ON's[1]
> > >
> > > I don't think that truncating len is worth the effort. The conversions being
> > > done should all 'work' At least corrupting users data in the same way as it
> > > used to... ;-) I'm ok with adding the WARN_ON's and I have modified the patch
> > > to do so while I work through the 0-day issues. (not sure what is going on
> > > there.)
> > >
> > > However, are we ok with adding the WARN_ON's given what Greg KH told me? This
> > > is a bit more critical than the PKS API in that it could result in corrupt
> > > data.
> >
> > zero_user_segments contains:
> >
> > BUG_ON(end1 > page_size(page) || end2 > page_size(page));
> >
> > These should be consistent. I think we've demonstrated that there is
> > no good option here.
>
> True, but these helpers are being deployed to many new locations where
> they were not used before.
So what's your preferred poison?
1. Corrupt random data in whatever's been mapped into the next page (which
is what the helpers currently do)
2. Copy less data than requested
3. Crash
4. Something else
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-08 22:32 ` Matthew Wilcox
@ 2020-12-08 22:45 ` Darrick J. Wong
2020-12-08 22:54 ` Matthew Wilcox
2020-12-08 23:40 ` Dan Williams
0 siblings, 2 replies; 30+ messages in thread
From: Darrick J. Wong @ 2020-12-08 22:45 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Dan Williams, Ira Weiny, Thomas Gleixner, Andrew Morton,
Dave Hansen, Christoph Hellwig, Al Viro, Eric Biggers,
Joonas Lahtinen, Linux Kernel Mailing List, linux-fsdevel
On Tue, Dec 08, 2020 at 10:32:34PM +0000, Matthew Wilcox wrote:
> On Tue, Dec 08, 2020 at 02:23:10PM -0800, Dan Williams wrote:
> > On Tue, Dec 8, 2020 at 1:51 PM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Tue, Dec 08, 2020 at 01:32:55PM -0800, Ira Weiny wrote:
> > > > On Mon, Dec 07, 2020 at 03:49:55PM -0800, Dan Williams wrote:
> > > > > On Mon, Dec 7, 2020 at 3:40 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > > > >
> > > > > > On Mon, Dec 07, 2020 at 03:34:44PM -0800, Dan Williams wrote:
> > > > > > > On Mon, Dec 7, 2020 at 3:27 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > > > > > >
> > > > > > > > On Mon, Dec 07, 2020 at 02:57:03PM -0800, ira.weiny@intel.com wrote:
> > > > > > > > > +static inline void memcpy_page(struct page *dst_page, size_t dst_off,
> > > > > > > > > + struct page *src_page, size_t src_off,
> > > > > > > > > + size_t len)
> > > > > > > > > +{
> > > > > > > > > + char *dst = kmap_local_page(dst_page);
> > > > > > > > > + char *src = kmap_local_page(src_page);
> > > > > > > >
> > > > > > > > I appreciate you've only moved these, but please add:
> > > > > > > >
> > > > > > > > BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
> > > > > > >
> > > > > > > I imagine it's not outside the realm of possibility that some driver
> > > > > > > on CONFIG_HIGHMEM=n is violating this assumption and getting away with
> > > > > > > it because kmap_atomic() of contiguous pages "just works (TM)".
> > > > > > > Shouldn't this WARN rather than BUG so that the user can report the
> > > > > > > buggy driver and not have a dead system?
> > > > > >
> > > > > > As opposed to (on a HIGHMEM=y system) silently corrupting data that
> > > > > > is on the next page of memory?
> > > > >
> > > > > Wouldn't it fault in HIGHMEM=y case? I guess not necessarily...
> > > > >
> > > > > > I suppose ideally ...
> > > > > >
> > > > > > if (WARN_ON(dst_off + len > PAGE_SIZE))
> > > > > > len = PAGE_SIZE - dst_off;
> > > > > > if (WARN_ON(src_off + len > PAGE_SIZE))
> > > > > > len = PAGE_SIZE - src_off;
> > > > > >
> > > > > > and then we just truncate the data of the offending caller instead of
> > > > > > corrupting innocent data that happens to be adjacent. Although that's
> > > > > > not ideal either ... I dunno, what's the least bad poison to drink here?
> > > > >
> > > > > Right, if the driver was relying on "corruption" for correct operation.
> > > > >
> > > > > If corruption actual were happening in practice wouldn't there have
> > > > > been screams by now? Again, not necessarily...
> > > > >
> > > > > At least with just plain WARN the kernel will start screaming on the
> > > > > user's behalf, and if it worked before it will keep working.
> > > >
> > > > So I decided to just sleep on this because I was recently told to not introduce
> > > > new WARN_ON's[1]
> > > >
> > > > I don't think that truncating len is worth the effort. The conversions being
> > > > done should all 'work' At least corrupting users data in the same way as it
> > > > used to... ;-) I'm ok with adding the WARN_ON's and I have modified the patch
> > > > to do so while I work through the 0-day issues. (not sure what is going on
> > > > there.)
> > > >
> > > > However, are we ok with adding the WARN_ON's given what Greg KH told me? This
> > > > is a bit more critical than the PKS API in that it could result in corrupt
> > > > data.
> > >
> > > zero_user_segments contains:
> > >
> > > BUG_ON(end1 > page_size(page) || end2 > page_size(page));
> > >
> > > These should be consistent. I think we've demonstrated that there is
> > > no good option here.
> >
> > True, but these helpers are being deployed to many new locations where
> > they were not used before.
>
> So what's your preferred poison?
>
> 1. Corrupt random data in whatever's been mapped into the next page (which
> is what the helpers currently do)
Please no.
> 2. Copy less data than requested
This sounds like the germination event for a research paper showing that
63% of callers never notice. ;)
> 3. Crash
Useful as a debug tool?
> 4. Something else
Return bytes copied like we do for writes that didn't quite work?
--D
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-08 22:45 ` Darrick J. Wong
@ 2020-12-08 22:54 ` Matthew Wilcox
2020-12-08 23:40 ` Dan Williams
1 sibling, 0 replies; 30+ messages in thread
From: Matthew Wilcox @ 2020-12-08 22:54 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Dan Williams, Ira Weiny, Thomas Gleixner, Andrew Morton,
Dave Hansen, Christoph Hellwig, Al Viro, Eric Biggers,
Joonas Lahtinen, Linux Kernel Mailing List, linux-fsdevel
On Tue, Dec 08, 2020 at 02:45:55PM -0800, Darrick J. Wong wrote:
> On Tue, Dec 08, 2020 at 10:32:34PM +0000, Matthew Wilcox wrote:
> > On Tue, Dec 08, 2020 at 02:23:10PM -0800, Dan Williams wrote:
> > > On Tue, Dec 8, 2020 at 1:51 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > >
> > > > On Tue, Dec 08, 2020 at 01:32:55PM -0800, Ira Weiny wrote:
> > > > > On Mon, Dec 07, 2020 at 03:49:55PM -0800, Dan Williams wrote:
> > > > > > On Mon, Dec 7, 2020 at 3:40 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > > > > >
> > > > > > > On Mon, Dec 07, 2020 at 03:34:44PM -0800, Dan Williams wrote:
> > > > > > > > On Mon, Dec 7, 2020 at 3:27 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > > > > > > >
> > > > > > > > > On Mon, Dec 07, 2020 at 02:57:03PM -0800, ira.weiny@intel.com wrote:
> > > > > > > > > > +static inline void memcpy_page(struct page *dst_page, size_t dst_off,
> > > > > > > > > > + struct page *src_page, size_t src_off,
> > > > > > > > > > + size_t len)
> > > > > > > > > > +{
> > > > > > > > > > + char *dst = kmap_local_page(dst_page);
> > > > > > > > > > + char *src = kmap_local_page(src_page);
> > > > > > > > >
> > > > > > > > > I appreciate you've only moved these, but please add:
> > > > > > > > >
> > > > > > > > > BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
> > > > > > > >
> > > > > > > > I imagine it's not outside the realm of possibility that some driver
> > > > > > > > on CONFIG_HIGHMEM=n is violating this assumption and getting away with
> > > > > > > > it because kmap_atomic() of contiguous pages "just works (TM)".
> > > > > > > > Shouldn't this WARN rather than BUG so that the user can report the
> > > > > > > > buggy driver and not have a dead system?
> > > > > > >
> > > > > > > As opposed to (on a HIGHMEM=y system) silently corrupting data that
> > > > > > > is on the next page of memory?
> > > > > >
> > > > > > Wouldn't it fault in HIGHMEM=y case? I guess not necessarily...
> > > > > >
> > > > > > > I suppose ideally ...
> > > > > > >
> > > > > > > if (WARN_ON(dst_off + len > PAGE_SIZE))
> > > > > > > len = PAGE_SIZE - dst_off;
> > > > > > > if (WARN_ON(src_off + len > PAGE_SIZE))
> > > > > > > len = PAGE_SIZE - src_off;
> > > > > > >
> > > > > > > and then we just truncate the data of the offending caller instead of
> > > > > > > corrupting innocent data that happens to be adjacent. Although that's
> > > > > > > not ideal either ... I dunno, what's the least bad poison to drink here?
> > > > > >
> > > > > > Right, if the driver was relying on "corruption" for correct operation.
> > > > > >
> > > > > > If corruption actual were happening in practice wouldn't there have
> > > > > > been screams by now? Again, not necessarily...
> > > > > >
> > > > > > At least with just plain WARN the kernel will start screaming on the
> > > > > > user's behalf, and if it worked before it will keep working.
> > > > >
> > > > > So I decided to just sleep on this because I was recently told to not introduce
> > > > > new WARN_ON's[1]
> > > > >
> > > > > I don't think that truncating len is worth the effort. The conversions being
> > > > > done should all 'work' At least corrupting users data in the same way as it
> > > > > used to... ;-) I'm ok with adding the WARN_ON's and I have modified the patch
> > > > > to do so while I work through the 0-day issues. (not sure what is going on
> > > > > there.)
> > > > >
> > > > > However, are we ok with adding the WARN_ON's given what Greg KH told me? This
> > > > > is a bit more critical than the PKS API in that it could result in corrupt
> > > > > data.
> > > >
> > > > zero_user_segments contains:
> > > >
> > > > BUG_ON(end1 > page_size(page) || end2 > page_size(page));
> > > >
> > > > These should be consistent. I think we've demonstrated that there is
> > > > no good option here.
> > >
> > > True, but these helpers are being deployed to many new locations where
> > > they were not used before.
> >
> > So what's your preferred poison?
> >
> > 1. Corrupt random data in whatever's been mapped into the next page (which
> > is what the helpers currently do)
>
> Please no.
>
> > 2. Copy less data than requested
>
> This sounds like the germination event for a research paper showing that
> 63% of callers never notice. ;)
>
> > 3. Crash
>
> Useful as a debug tool?
>
> > 4. Something else
>
> Return bytes copied like we do for writes that didn't quite work?
... to learn that 87% of callers never check the return value, 10%
of them do the wrong thing and the remainder have never been tested?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-08 22:45 ` Darrick J. Wong
2020-12-08 22:54 ` Matthew Wilcox
@ 2020-12-08 23:40 ` Dan Williams
2020-12-09 2:22 ` Ira Weiny
1 sibling, 1 reply; 30+ messages in thread
From: Dan Williams @ 2020-12-08 23:40 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Matthew Wilcox, Ira Weiny, Thomas Gleixner, Andrew Morton,
Dave Hansen, Christoph Hellwig, Al Viro, Eric Biggers,
Joonas Lahtinen, Linux Kernel Mailing List, linux-fsdevel
On Tue, Dec 8, 2020 at 2:49 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
[..]
> > So what's your preferred poison?
> >
> > 1. Corrupt random data in whatever's been mapped into the next page (which
> > is what the helpers currently do)
>
> Please no.
My assertion is that the kernel can't know it's corruption, it can
only know that the driver is abusing the API. So over-copy and WARN
seems better than violently regress by crashing what might have been
working silently before.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-08 23:40 ` Dan Williams
@ 2020-12-09 2:22 ` Ira Weiny
2020-12-09 4:03 ` Matthew Wilcox
0 siblings, 1 reply; 30+ messages in thread
From: Ira Weiny @ 2020-12-09 2:22 UTC (permalink / raw)
To: Dan Williams
Cc: Darrick J. Wong, Matthew Wilcox, Thomas Gleixner, Andrew Morton,
Dave Hansen, Christoph Hellwig, Al Viro, Eric Biggers,
Joonas Lahtinen, Linux Kernel Mailing List, linux-fsdevel
On Tue, Dec 08, 2020 at 03:40:52PM -0800, Dan Williams wrote:
> On Tue, Dec 8, 2020 at 2:49 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> [..]
> > > So what's your preferred poison?
> > >
> > > 1. Corrupt random data in whatever's been mapped into the next page (which
> > > is what the helpers currently do)
> >
> > Please no.
>
> My assertion is that the kernel can't know it's corruption, it can
> only know that the driver is abusing the API. So over-copy and WARN
> seems better than violently regress by crashing what might have been
> working silently before.
Right now we have a mixed bag. zero_user() [and it's variants, circa 2008]
does a BUG_ON.[0] While the other ones do nothing; clear_highpage(),
clear_user_highpage(), copy_user_highpage(), and copy_highpage().
While continuing to audit the code I don't see any users who would violating
the API with a simple conversion of the code. The calls which I have worked on
[which is many at this point] all have checks in place which are well aware of
page boundaries.
Therefore, I tend to agree with Dan that if anything is to be done it should be
a WARN_ON() which is only going to throw an error that something has probably
been wrong all along and should be fixed but continue running as before.
BUG_ON() is a very big hammer. And I don't think that Linus is going to
appreciate a BUG_ON here.[1] Callers of this API should be well aware that
they are operating on a page and that specifying parameters beyond the bounds
of a page are going to have bad consequences...
Furthermore, I'm still leery of adding the WARN_ON's because Greg KH says many
people will be converting them to BUG_ON's via panic-on-warn anyway. But at
least that is their choice.
FWIW I think this is a 'bad BUG_ON' use because we are "checking something that
we know we might be getting wrong".[1] And because, "BUG() is only good for
something that never happens and that we really have no other option for".[2]
IMO, These calls are like memcpy/memmove. memcpy/memmove don't validate bounds
and developers have lived with those constructs for a long time.
Ira
[0] BTW, After writing this email, with various URL research, I think this
BUG_ON() is also probably wrong...
[1]
<quote>
...
It's [BUG_ON] not a "let's check that
everybody did things right", it's a "this is a major design rule in
this core code".
...
</quote>
-- Linus (https://lkml.org/lkml/2016/10/4/337)
[2] https://yarchive.net/comp/linux/BUG.html
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-09 2:22 ` Ira Weiny
@ 2020-12-09 4:03 ` Matthew Wilcox
2020-12-09 19:47 ` Dan Williams
0 siblings, 1 reply; 30+ messages in thread
From: Matthew Wilcox @ 2020-12-09 4:03 UTC (permalink / raw)
To: Ira Weiny
Cc: Dan Williams, Darrick J. Wong, Thomas Gleixner, Andrew Morton,
Dave Hansen, Christoph Hellwig, Al Viro, Eric Biggers,
Joonas Lahtinen, Linux Kernel Mailing List, linux-fsdevel
On Tue, Dec 08, 2020 at 06:22:50PM -0800, Ira Weiny wrote:
> Right now we have a mixed bag. zero_user() [and it's variants, circa 2008]
> does a BUG_ON.[0] While the other ones do nothing; clear_highpage(),
> clear_user_highpage(), copy_user_highpage(), and copy_highpage().
Erm, those functions operate on the entire PAGE_SIZE. There's nothing
for them to check.
> While continuing to audit the code I don't see any users who would violating
> the API with a simple conversion of the code. The calls which I have worked on
> [which is many at this point] all have checks in place which are well aware of
> page boundaries.
Oh good, then this BUG_ON won't trigger.
> Therefore, I tend to agree with Dan that if anything is to be done it should be
> a WARN_ON() which is only going to throw an error that something has probably
> been wrong all along and should be fixed but continue running as before.
Silent data corruption is for ever. Are you absolutely sure nobody has
done:
page = alloc_pages(GFP_HIGHUSER_MOVABLE, 3);
memcpy_to_page(page, PAGE_SIZE * 2, p, PAGE_SIZE * 2);
because that will work fine if the pages come from ZONE_NORMAL and fail
miserably if they came from ZONE_HIGHMEM.
> FWIW I think this is a 'bad BUG_ON' use because we are "checking something that
> we know we might be getting wrong".[1] And because, "BUG() is only good for
> something that never happens and that we really have no other option for".[2]
BUG() is our only option here. Both limiting how much we copy or
copying the requested amount result in data corruption or leaking
information to a process that isn't supposed to see it.
What Linus is railing against is the developers who say "Oh, I don't
know what to do here, I'll just BUG()". That's not the case here.
We've thought about it. We've discussed it. There's NO GOOD OPTION.
Unless you want to do the moral equivalent of this:
http://git.infradead.org/users/willy/pagecache.git/commitdiff/d2417516bd8b3dd1db096a9b040b0264d8052339
I think that would look something like this ...
void memcpy_to_page(struct page *page, size_t offset, const char *from,
size_t len)
{
page += offset / PAGE_SIZE;
offset %= PAGE_SIZE;
while (len) {
char *to = kmap_atomic(page);
size_t bytes = min(len, PAGE_SIZE - offset);
memcpy(to + offset, from, len);
kunmap_atomic(to);
len -= bytes;
offset = 0;
page++;
}
}
Now 32-bit highmem will do the same thing as 64-bit for my example above,
just more slowly. Untested, obviously.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-09 4:03 ` Matthew Wilcox
@ 2020-12-09 19:47 ` Dan Williams
2020-12-09 20:14 ` Matthew Wilcox
0 siblings, 1 reply; 30+ messages in thread
From: Dan Williams @ 2020-12-09 19:47 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Ira Weiny, Darrick J. Wong, Thomas Gleixner, Andrew Morton,
Dave Hansen, Christoph Hellwig, Al Viro, Eric Biggers,
Joonas Lahtinen, Linux Kernel Mailing List, linux-fsdevel
On Tue, Dec 8, 2020 at 8:03 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Tue, Dec 08, 2020 at 06:22:50PM -0800, Ira Weiny wrote:
> > Right now we have a mixed bag. zero_user() [and it's variants, circa 2008]
> > does a BUG_ON.[0] While the other ones do nothing; clear_highpage(),
> > clear_user_highpage(), copy_user_highpage(), and copy_highpage().
>
> Erm, those functions operate on the entire PAGE_SIZE. There's nothing
> for them to check.
>
> > While continuing to audit the code I don't see any users who would violating
> > the API with a simple conversion of the code. The calls which I have worked on
> > [which is many at this point] all have checks in place which are well aware of
> > page boundaries.
>
> Oh good, then this BUG_ON won't trigger.
>
> > Therefore, I tend to agree with Dan that if anything is to be done it should be
> > a WARN_ON() which is only going to throw an error that something has probably
> > been wrong all along and should be fixed but continue running as before.
>
> Silent data corruption is for ever. Are you absolutely sure nobody has
> done:
>
> page = alloc_pages(GFP_HIGHUSER_MOVABLE, 3);
> memcpy_to_page(page, PAGE_SIZE * 2, p, PAGE_SIZE * 2);
>
> because that will work fine if the pages come from ZONE_NORMAL and fail
> miserably if they came from ZONE_HIGHMEM.
...and violently regress with the BUG_ON.
The question to me is: which is more likely that any bad usages have
been covered up by being limited to ZONE_NORMAL / 64-bit only, or that
silent data corruption has been occurring with no ill effects?
> > FWIW I think this is a 'bad BUG_ON' use because we are "checking something that
> > we know we might be getting wrong".[1] And because, "BUG() is only good for
> > something that never happens and that we really have no other option for".[2]
>
> BUG() is our only option here. Both limiting how much we copy or
> copying the requested amount result in data corruption or leaking
> information to a process that isn't supposed to see it.
At a minimum I think this should be debated in a follow on patch to
add assertion checking where there was none before. There is no
evidence of a page being overrun in the audit Ira performed.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-09 19:47 ` Dan Williams
@ 2020-12-09 20:14 ` Matthew Wilcox
2020-12-09 20:19 ` Dan Williams
2020-12-10 5:35 ` Ira Weiny
0 siblings, 2 replies; 30+ messages in thread
From: Matthew Wilcox @ 2020-12-09 20:14 UTC (permalink / raw)
To: Dan Williams
Cc: Ira Weiny, Darrick J. Wong, Thomas Gleixner, Andrew Morton,
Dave Hansen, Christoph Hellwig, Al Viro, Eric Biggers,
Joonas Lahtinen, Linux Kernel Mailing List, linux-fsdevel
On Wed, Dec 09, 2020 at 11:47:56AM -0800, Dan Williams wrote:
> On Tue, Dec 8, 2020 at 8:03 PM Matthew Wilcox <willy@infradead.org> wrote:
> > On Tue, Dec 08, 2020 at 06:22:50PM -0800, Ira Weiny wrote:
> > > Therefore, I tend to agree with Dan that if anything is to be done it should be
> > > a WARN_ON() which is only going to throw an error that something has probably
> > > been wrong all along and should be fixed but continue running as before.
> >
> > Silent data corruption is for ever. Are you absolutely sure nobody has
> > done:
> >
> > page = alloc_pages(GFP_HIGHUSER_MOVABLE, 3);
> > memcpy_to_page(page, PAGE_SIZE * 2, p, PAGE_SIZE * 2);
> >
> > because that will work fine if the pages come from ZONE_NORMAL and fail
> > miserably if they came from ZONE_HIGHMEM.
>
> ...and violently regress with the BUG_ON.
... which is what we want, no?
> The question to me is: which is more likely that any bad usages have
> been covered up by being limited to ZONE_NORMAL / 64-bit only, or that
> silent data corruption has been occurring with no ill effects?
I wouldn't be at all surprised to learn that there is silent data
corruption on 32-bit systems with HIGHMEM. Would you? How much testing
do you do on 32-bit HIGHMEM systems?
Actually, I wouldn't be at all surprised if we can hit this problem today.
Look at this:
size_t _copy_from_iter(void *addr, size_t bytes, struct iov_iter *i)
{
char *to = addr;
if (unlikely(iov_iter_is_pipe(i))) {
WARN_ON(1);
return 0;
}
if (iter_is_iovec(i))
might_fault();
iterate_and_advance(i, bytes, v,
copyin((to += v.iov_len) - v.iov_len, v.iov_base, v.iov_len),
memcpy_from_page((to += v.bv_len) - v.bv_len, v.bv_page,
v.bv_offset, v.bv_len),
memcpy((to += v.iov_len) - v.iov_len, v.iov_base, v.iov_len)
)
return bytes;
}
EXPORT_SYMBOL(_copy_from_iter);
There's a lot of macrology in there, so for those following along who
aren't familiar with the iov_iter code, if the iter is operating on a
bvec, then iterate_and_advance() will call memcpy_from_page(), passing
it the bv_page, bv_offset and bv_len stored in the bvec. Since 2019,
Linux has supported multipage bvecs (commit 07173c3ec276). So bv_len
absolutely *can* be > PAGE_SIZE.
Does this ever happen in practice? I have no idea; I don't know whether
any multipage BIOs are currently handed to copy_from_iter(). But I
have no confidence in your audit if you didn't catch this one.
> > > FWIW I think this is a 'bad BUG_ON' use because we are "checking something that
> > > we know we might be getting wrong".[1] And because, "BUG() is only good for
> > > something that never happens and that we really have no other option for".[2]
> >
> > BUG() is our only option here. Both limiting how much we copy or
> > copying the requested amount result in data corruption or leaking
> > information to a process that isn't supposed to see it.
>
> At a minimum I think this should be debated in a follow on patch to
> add assertion checking where there was none before. There is no
> evidence of a page being overrun in the audit Ira performed.
If we put in into a separate patch, someone will suggest backing out the
patch which tells us that there's a problem. You know, like this guy ...
https://lore.kernel.org/linux-mm/CAPcyv4jNVroYmirzKw_=CsEixOEACdL3M1Wc4xjd_TFv3h+o8Q@mail.gmail.com/
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-09 20:14 ` Matthew Wilcox
@ 2020-12-09 20:19 ` Dan Williams
2020-12-10 5:35 ` Ira Weiny
1 sibling, 0 replies; 30+ messages in thread
From: Dan Williams @ 2020-12-09 20:19 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Ira Weiny, Darrick J. Wong, Thomas Gleixner, Andrew Morton,
Dave Hansen, Christoph Hellwig, Al Viro, Eric Biggers,
Joonas Lahtinen, Linux Kernel Mailing List, linux-fsdevel
On Wed, Dec 9, 2020 at 12:14 PM Matthew Wilcox <willy@infradead.org> wrote:
[..]
> If we put in into a separate patch, someone will suggest backing out the
> patch which tells us that there's a problem. You know, like this guy ...
> https://lore.kernel.org/linux-mm/CAPcyv4jNVroYmirzKw_=CsEixOEACdL3M1Wc4xjd_TFv3h+o8Q@mail.gmail.com/
...and that person, like in this case, will be told 'no' and go on to
find / fix the real problem.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-09 20:14 ` Matthew Wilcox
2020-12-09 20:19 ` Dan Williams
@ 2020-12-10 5:35 ` Ira Weiny
1 sibling, 0 replies; 30+ messages in thread
From: Ira Weiny @ 2020-12-10 5:35 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Dan Williams, Darrick J. Wong, Thomas Gleixner, Andrew Morton,
Dave Hansen, Christoph Hellwig, Al Viro, Eric Biggers,
Joonas Lahtinen, Linux Kernel Mailing List, linux-fsdevel
On Wed, Dec 09, 2020 at 08:14:15PM +0000, Matthew Wilcox wrote:
> On Wed, Dec 09, 2020 at 11:47:56AM -0800, Dan Williams wrote:
> > On Tue, Dec 8, 2020 at 8:03 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > On Tue, Dec 08, 2020 at 06:22:50PM -0800, Ira Weiny wrote:
> > > > Therefore, I tend to agree with Dan that if anything is to be done it should be
> > > > a WARN_ON() which is only going to throw an error that something has probably
> > > > been wrong all along and should be fixed but continue running as before.
> > >
> > > Silent data corruption is for ever. Are you absolutely sure nobody has
> > > done:
> > >
> > > page = alloc_pages(GFP_HIGHUSER_MOVABLE, 3);
> > > memcpy_to_page(page, PAGE_SIZE * 2, p, PAGE_SIZE * 2);
> > >
> > > because that will work fine if the pages come from ZONE_NORMAL and fail
> > > miserably if they came from ZONE_HIGHMEM.
> >
> > ...and violently regress with the BUG_ON.
>
> ... which is what we want, no?
>
> > The question to me is: which is more likely that any bad usages have
> > been covered up by being limited to ZONE_NORMAL / 64-bit only, or that
> > silent data corruption has been occurring with no ill effects?
>
> I wouldn't be at all surprised to learn that there is silent data
> corruption on 32-bit systems with HIGHMEM. Would you? How much testing
> do you do on 32-bit HIGHMEM systems?
>
> Actually, I wouldn't be at all surprised if we can hit this problem today.
> Look at this:
>
> size_t _copy_from_iter(void *addr, size_t bytes, struct iov_iter *i)
> {
> char *to = addr;
> if (unlikely(iov_iter_is_pipe(i))) {
> WARN_ON(1);
> return 0;
> }
> if (iter_is_iovec(i))
> might_fault();
> iterate_and_advance(i, bytes, v,
> copyin((to += v.iov_len) - v.iov_len, v.iov_base, v.iov_len),
> memcpy_from_page((to += v.bv_len) - v.bv_len, v.bv_page,
> v.bv_offset, v.bv_len),
> memcpy((to += v.iov_len) - v.iov_len, v.iov_base, v.iov_len)
> )
>
> return bytes;
> }
> EXPORT_SYMBOL(_copy_from_iter);
>
> There's a lot of macrology in there, so for those following along who
> aren't familiar with the iov_iter code, if the iter is operating on a
> bvec, then iterate_and_advance() will call memcpy_from_page(), passing
> it the bv_page, bv_offset and bv_len stored in the bvec. Since 2019,
> Linux has supported multipage bvecs (commit 07173c3ec276). So bv_len
> absolutely *can* be > PAGE_SIZE.
>
> Does this ever happen in practice? I have no idea; I don't know whether
> any multipage BIOs are currently handed to copy_from_iter(). But I
> have no confidence in your audit if you didn't catch this one.
Ah... This call site has been there since 2014 and is not a new caller I have
been 'auditing'.[1]
>
> > > > FWIW I think this is a 'bad BUG_ON' use because we are "checking something that
> > > > we know we might be getting wrong".[1] And because, "BUG() is only good for
> > > > something that never happens and that we really have no other option for".[2]
> > >
> > > BUG() is our only option here. Both limiting how much we copy or
> > > copying the requested amount result in data corruption or leaking
> > > information to a process that isn't supposed to see it.
> >
> > At a minimum I think this should be debated in a follow on patch to
> > add assertion checking where there was none before. There is no
> > evidence of a page being overrun in the audit Ira performed.
>
> If we put in into a separate patch, someone will suggest backing out the
> patch which tells us that there's a problem. You know, like this guy ...
> https://lore.kernel.org/linux-mm/CAPcyv4jNVroYmirzKw_=CsEixOEACdL3M1Wc4xjd_TFv3h+o8Q@mail.gmail.com/
I'm not following this. Regardless I've already added the BUG_ON's.
Ira
[1]
commit 0dbca9a4b5d69a7e4b8c1d55b98312fcd9aafcf7
Author: Al Viro <viro@zeniv.linux.org.uk>
Date: Thu Nov 27 14:26:43 2014 -0500
iov_iter.c: convert copy_from_iter() to iterate_and_advance
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-08 21:32 ` Ira Weiny
2020-12-08 21:50 ` Matthew Wilcox
@ 2020-12-08 22:21 ` Dan Williams
1 sibling, 0 replies; 30+ messages in thread
From: Dan Williams @ 2020-12-08 22:21 UTC (permalink / raw)
To: Ira Weiny
Cc: Matthew Wilcox, Thomas Gleixner, Andrew Morton, Dave Hansen,
Christoph Hellwig, Al Viro, Eric Biggers, Joonas Lahtinen,
Linux Kernel Mailing List, linux-fsdevel
On Tue, Dec 8, 2020 at 1:33 PM Ira Weiny <ira.weiny@intel.com> wrote:
>
> On Mon, Dec 07, 2020 at 03:49:55PM -0800, Dan Williams wrote:
> > On Mon, Dec 7, 2020 at 3:40 PM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Mon, Dec 07, 2020 at 03:34:44PM -0800, Dan Williams wrote:
> > > > On Mon, Dec 7, 2020 at 3:27 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > > >
> > > > > On Mon, Dec 07, 2020 at 02:57:03PM -0800, ira.weiny@intel.com wrote:
> > > > > > +static inline void memcpy_page(struct page *dst_page, size_t dst_off,
> > > > > > + struct page *src_page, size_t src_off,
> > > > > > + size_t len)
> > > > > > +{
> > > > > > + char *dst = kmap_local_page(dst_page);
> > > > > > + char *src = kmap_local_page(src_page);
> > > > >
> > > > > I appreciate you've only moved these, but please add:
> > > > >
> > > > > BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
> > > >
> > > > I imagine it's not outside the realm of possibility that some driver
> > > > on CONFIG_HIGHMEM=n is violating this assumption and getting away with
> > > > it because kmap_atomic() of contiguous pages "just works (TM)".
> > > > Shouldn't this WARN rather than BUG so that the user can report the
> > > > buggy driver and not have a dead system?
> > >
> > > As opposed to (on a HIGHMEM=y system) silently corrupting data that
> > > is on the next page of memory?
> >
> > Wouldn't it fault in HIGHMEM=y case? I guess not necessarily...
> >
> > > I suppose ideally ...
> > >
> > > if (WARN_ON(dst_off + len > PAGE_SIZE))
> > > len = PAGE_SIZE - dst_off;
> > > if (WARN_ON(src_off + len > PAGE_SIZE))
> > > len = PAGE_SIZE - src_off;
> > >
> > > and then we just truncate the data of the offending caller instead of
> > > corrupting innocent data that happens to be adjacent. Although that's
> > > not ideal either ... I dunno, what's the least bad poison to drink here?
> >
> > Right, if the driver was relying on "corruption" for correct operation.
> >
> > If corruption actual were happening in practice wouldn't there have
> > been screams by now? Again, not necessarily...
> >
> > At least with just plain WARN the kernel will start screaming on the
> > user's behalf, and if it worked before it will keep working.
>
> So I decided to just sleep on this because I was recently told to not introduce
> new WARN_ON's[1]
>
> I don't think that truncating len is worth the effort. The conversions being
> done should all 'work' At least corrupting users data in the same way as it
> used to... ;-) I'm ok with adding the WARN_ON's and I have modified the patch
> to do so while I work through the 0-day issues. (not sure what is going on
> there.)
>
> However, are we ok with adding the WARN_ON's given what Greg KH told me? This
> is a bit more critical than the PKS API in that it could result in corrupt
> data.
That situation was a bit different in my mind because the default
fallback stub path has typically never had WARN_ON even if it's never
supposed to be called.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-07 22:57 ` [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core ira.weiny
@ 2020-12-08 0:40 ` kernel test robot
2020-12-08 0:40 ` kernel test robot
` (2 subsequent siblings)
3 siblings, 0 replies; 30+ messages in thread
From: kernel test robot @ 2020-12-08 0:40 UTC (permalink / raw)
To: ira.weiny, Thomas Gleixner, Andrew Morton
Cc: kbuild-all, Linux Memory Management List, Ira Weiny, Dave Hansen,
Matthew Wilcox, Christoph Hellwig, Dan Williams, Al Viro,
Eric Biggers
[-- Attachment #1: Type: text/plain, Size: 11401 bytes --]
Hi,
I love your patch! Perhaps something to improve:
[auto build test WARNING on hch-configfs/for-next]
[also build test WARNING on linus/master v5.10-rc7]
[cannot apply to hnaz-linux-mm/master next-20201207]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/ira-weiny-intel-com/Lift-memcpy_-to-from-_page-to-core/20201208-070017
base: git://git.infradead.org/users/hch/configfs.git for-next
config: riscv-nommu_k210_defconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/23e6d3f08a315c6e70fde3d63a275c91e1dcb0ee
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review ira-weiny-intel-com/Lift-memcpy_-to-from-_page-to-core/20201208-070017
git checkout 23e6d3f08a315c6e70fde3d63a275c91e1dcb0ee
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=riscv
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from drivers/char/mem.c:24:
include/linux/highmem.h: In function 'clear_user_highpage':
include/linux/highmem.h:229:15: error: implicit declaration of function 'kmap_local_page'; did you mean 'kmap_to_page'? [-Werror=implicit-function-declaration]
229 | void *addr = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
| kmap_to_page
include/linux/highmem.h:229:15: warning: initialization of 'void *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
include/linux/highmem.h:231:2: error: implicit declaration of function 'kunmap_local' [-Werror=implicit-function-declaration]
231 | kunmap_local(addr);
| ^~~~~~~~~~~~
include/linux/highmem.h: In function 'clear_highpage':
include/linux/highmem.h:282:16: warning: initialization of 'void *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
282 | void *kaddr = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
include/linux/highmem.h: In function 'zero_user_segments':
include/linux/highmem.h:291:16: warning: initialization of 'void *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
291 | void *kaddr = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
include/linux/highmem.h: In function 'copy_user_highpage':
include/linux/highmem.h:324:8: warning: assignment to 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
324 | vfrom = kmap_local_page(from);
| ^
include/linux/highmem.h:325:6: warning: assignment to 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
325 | vto = kmap_local_page(to);
| ^
include/linux/highmem.h: In function 'copy_highpage':
include/linux/highmem.h:339:8: warning: assignment to 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
339 | vfrom = kmap_local_page(from);
| ^
include/linux/highmem.h:340:6: warning: assignment to 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
340 | vto = kmap_local_page(to);
| ^
In file included from include/linux/blkdev.h:14,
from include/linux/backing-dev.h:15,
from drivers/char/mem.c:25:
include/linux/pagemap.h: In function 'memcpy_page':
>> include/linux/pagemap.h:1036:14: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1036 | char *dst = kmap_local_page(dst_page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h:1037:14: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1037 | char *src = kmap_local_page(src_page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h: In function 'memmove_page':
include/linux/pagemap.h:1047:14: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1047 | char *dst = kmap_local_page(dst_page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h:1048:14: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1048 | char *src = kmap_local_page(src_page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h: In function 'memcpy_from_page':
include/linux/pagemap.h:1056:15: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1056 | char *from = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h: In function 'memcpy_to_page':
include/linux/pagemap.h:1063:13: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1063 | char *to = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h: In function 'memset_page':
include/linux/pagemap.h:1070:15: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1070 | char *addr = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
--
In file included from include/linux/pagemap.h:11,
from include/linux/blkdev.h:14,
from include/linux/blk-cgroup.h:23,
from include/linux/writeback.h:14,
from include/trace/events/random.h:8,
from drivers/char/random.c:348:
include/linux/highmem.h: In function 'clear_user_highpage':
include/linux/highmem.h:229:15: error: implicit declaration of function 'kmap_local_page'; did you mean 'kmap_to_page'? [-Werror=implicit-function-declaration]
229 | void *addr = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
| kmap_to_page
include/linux/highmem.h:229:15: warning: initialization of 'void *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
include/linux/highmem.h:231:2: error: implicit declaration of function 'kunmap_local' [-Werror=implicit-function-declaration]
231 | kunmap_local(addr);
| ^~~~~~~~~~~~
include/linux/highmem.h: In function 'clear_highpage':
include/linux/highmem.h:282:16: warning: initialization of 'void *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
282 | void *kaddr = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
include/linux/highmem.h: In function 'zero_user_segments':
include/linux/highmem.h:291:16: warning: initialization of 'void *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
291 | void *kaddr = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
include/linux/highmem.h: In function 'copy_user_highpage':
include/linux/highmem.h:324:8: warning: assignment to 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
324 | vfrom = kmap_local_page(from);
| ^
include/linux/highmem.h:325:6: warning: assignment to 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
325 | vto = kmap_local_page(to);
| ^
include/linux/highmem.h: In function 'copy_highpage':
include/linux/highmem.h:339:8: warning: assignment to 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
339 | vfrom = kmap_local_page(from);
| ^
include/linux/highmem.h:340:6: warning: assignment to 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
340 | vto = kmap_local_page(to);
| ^
In file included from include/linux/blkdev.h:14,
from include/linux/blk-cgroup.h:23,
from include/linux/writeback.h:14,
from include/trace/events/random.h:8,
from drivers/char/random.c:348:
include/linux/pagemap.h: In function 'memcpy_page':
>> include/linux/pagemap.h:1036:14: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1036 | char *dst = kmap_local_page(dst_page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h:1037:14: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1037 | char *src = kmap_local_page(src_page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h: In function 'memmove_page':
include/linux/pagemap.h:1047:14: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1047 | char *dst = kmap_local_page(dst_page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h:1048:14: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1048 | char *src = kmap_local_page(src_page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h: In function 'memcpy_from_page':
include/linux/pagemap.h:1056:15: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1056 | char *from = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h: In function 'memcpy_to_page':
include/linux/pagemap.h:1063:13: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1063 | char *to = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h: In function 'memset_page':
include/linux/pagemap.h:1070:15: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1070 | char *addr = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
drivers/char/random.c: At top level:
drivers/char/random.c:2296:6: warning: no previous prototype for 'add_hwgenerator_randomness' [-Wmissing-prototypes]
2296 | void add_hwgenerator_randomness(const char *buffer, size_t count,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +1036 include/linux/pagemap.h
1031
1032 static inline void memcpy_page(struct page *dst_page, size_t dst_off,
1033 struct page *src_page, size_t src_off,
1034 size_t len)
1035 {
> 1036 char *dst = kmap_local_page(dst_page);
1037 char *src = kmap_local_page(src_page);
1038 memcpy(dst + dst_off, src + src_off, len);
1039 kunmap_local(src);
1040 kunmap_local(dst);
1041 }
1042
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6505 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
@ 2020-12-08 0:40 ` kernel test robot
0 siblings, 0 replies; 30+ messages in thread
From: kernel test robot @ 2020-12-08 0:40 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 11588 bytes --]
Hi,
I love your patch! Perhaps something to improve:
[auto build test WARNING on hch-configfs/for-next]
[also build test WARNING on linus/master v5.10-rc7]
[cannot apply to hnaz-linux-mm/master next-20201207]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/ira-weiny-intel-com/Lift-memcpy_-to-from-_page-to-core/20201208-070017
base: git://git.infradead.org/users/hch/configfs.git for-next
config: riscv-nommu_k210_defconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/23e6d3f08a315c6e70fde3d63a275c91e1dcb0ee
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review ira-weiny-intel-com/Lift-memcpy_-to-from-_page-to-core/20201208-070017
git checkout 23e6d3f08a315c6e70fde3d63a275c91e1dcb0ee
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=riscv
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from drivers/char/mem.c:24:
include/linux/highmem.h: In function 'clear_user_highpage':
include/linux/highmem.h:229:15: error: implicit declaration of function 'kmap_local_page'; did you mean 'kmap_to_page'? [-Werror=implicit-function-declaration]
229 | void *addr = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
| kmap_to_page
include/linux/highmem.h:229:15: warning: initialization of 'void *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
include/linux/highmem.h:231:2: error: implicit declaration of function 'kunmap_local' [-Werror=implicit-function-declaration]
231 | kunmap_local(addr);
| ^~~~~~~~~~~~
include/linux/highmem.h: In function 'clear_highpage':
include/linux/highmem.h:282:16: warning: initialization of 'void *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
282 | void *kaddr = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
include/linux/highmem.h: In function 'zero_user_segments':
include/linux/highmem.h:291:16: warning: initialization of 'void *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
291 | void *kaddr = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
include/linux/highmem.h: In function 'copy_user_highpage':
include/linux/highmem.h:324:8: warning: assignment to 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
324 | vfrom = kmap_local_page(from);
| ^
include/linux/highmem.h:325:6: warning: assignment to 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
325 | vto = kmap_local_page(to);
| ^
include/linux/highmem.h: In function 'copy_highpage':
include/linux/highmem.h:339:8: warning: assignment to 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
339 | vfrom = kmap_local_page(from);
| ^
include/linux/highmem.h:340:6: warning: assignment to 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
340 | vto = kmap_local_page(to);
| ^
In file included from include/linux/blkdev.h:14,
from include/linux/backing-dev.h:15,
from drivers/char/mem.c:25:
include/linux/pagemap.h: In function 'memcpy_page':
>> include/linux/pagemap.h:1036:14: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1036 | char *dst = kmap_local_page(dst_page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h:1037:14: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1037 | char *src = kmap_local_page(src_page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h: In function 'memmove_page':
include/linux/pagemap.h:1047:14: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1047 | char *dst = kmap_local_page(dst_page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h:1048:14: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1048 | char *src = kmap_local_page(src_page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h: In function 'memcpy_from_page':
include/linux/pagemap.h:1056:15: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1056 | char *from = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h: In function 'memcpy_to_page':
include/linux/pagemap.h:1063:13: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1063 | char *to = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h: In function 'memset_page':
include/linux/pagemap.h:1070:15: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1070 | char *addr = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
--
In file included from include/linux/pagemap.h:11,
from include/linux/blkdev.h:14,
from include/linux/blk-cgroup.h:23,
from include/linux/writeback.h:14,
from include/trace/events/random.h:8,
from drivers/char/random.c:348:
include/linux/highmem.h: In function 'clear_user_highpage':
include/linux/highmem.h:229:15: error: implicit declaration of function 'kmap_local_page'; did you mean 'kmap_to_page'? [-Werror=implicit-function-declaration]
229 | void *addr = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
| kmap_to_page
include/linux/highmem.h:229:15: warning: initialization of 'void *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
include/linux/highmem.h:231:2: error: implicit declaration of function 'kunmap_local' [-Werror=implicit-function-declaration]
231 | kunmap_local(addr);
| ^~~~~~~~~~~~
include/linux/highmem.h: In function 'clear_highpage':
include/linux/highmem.h:282:16: warning: initialization of 'void *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
282 | void *kaddr = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
include/linux/highmem.h: In function 'zero_user_segments':
include/linux/highmem.h:291:16: warning: initialization of 'void *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
291 | void *kaddr = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
include/linux/highmem.h: In function 'copy_user_highpage':
include/linux/highmem.h:324:8: warning: assignment to 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
324 | vfrom = kmap_local_page(from);
| ^
include/linux/highmem.h:325:6: warning: assignment to 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
325 | vto = kmap_local_page(to);
| ^
include/linux/highmem.h: In function 'copy_highpage':
include/linux/highmem.h:339:8: warning: assignment to 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
339 | vfrom = kmap_local_page(from);
| ^
include/linux/highmem.h:340:6: warning: assignment to 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
340 | vto = kmap_local_page(to);
| ^
In file included from include/linux/blkdev.h:14,
from include/linux/blk-cgroup.h:23,
from include/linux/writeback.h:14,
from include/trace/events/random.h:8,
from drivers/char/random.c:348:
include/linux/pagemap.h: In function 'memcpy_page':
>> include/linux/pagemap.h:1036:14: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1036 | char *dst = kmap_local_page(dst_page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h:1037:14: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1037 | char *src = kmap_local_page(src_page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h: In function 'memmove_page':
include/linux/pagemap.h:1047:14: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1047 | char *dst = kmap_local_page(dst_page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h:1048:14: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1048 | char *src = kmap_local_page(src_page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h: In function 'memcpy_from_page':
include/linux/pagemap.h:1056:15: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1056 | char *from = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h: In function 'memcpy_to_page':
include/linux/pagemap.h:1063:13: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1063 | char *to = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
include/linux/pagemap.h: In function 'memset_page':
include/linux/pagemap.h:1070:15: warning: initialization of 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
1070 | char *addr = kmap_local_page(page);
| ^~~~~~~~~~~~~~~
drivers/char/random.c: At top level:
drivers/char/random.c:2296:6: warning: no previous prototype for 'add_hwgenerator_randomness' [-Wmissing-prototypes]
2296 | void add_hwgenerator_randomness(const char *buffer, size_t count,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +1036 include/linux/pagemap.h
1031
1032 static inline void memcpy_page(struct page *dst_page, size_t dst_off,
1033 struct page *src_page, size_t src_off,
1034 size_t len)
1035 {
> 1036 char *dst = kmap_local_page(dst_page);
1037 char *src = kmap_local_page(src_page);
1038 memcpy(dst + dst_off, src + src_off, len);
1039 kunmap_local(src);
1040 kunmap_local(dst);
1041 }
1042
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 6505 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-07 22:57 ` [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core ira.weiny
@ 2020-12-08 1:09 ` kernel test robot
2020-12-08 0:40 ` kernel test robot
` (2 subsequent siblings)
3 siblings, 0 replies; 30+ messages in thread
From: kernel test robot @ 2020-12-08 1:09 UTC (permalink / raw)
To: ira.weiny, Thomas Gleixner, Andrew Morton
Cc: kbuild-all, clang-built-linux, Linux Memory Management List,
Ira Weiny, Dave Hansen, Matthew Wilcox, Christoph Hellwig,
Dan Williams, Al Viro, Eric Biggers
[-- Attachment #1: Type: text/plain, Size: 22540 bytes --]
Hi,
I love your patch! Yet something to improve:
[auto build test ERROR on hch-configfs/for-next]
[also build test ERROR on linus/master v5.10-rc7]
[cannot apply to hnaz-linux-mm/master next-20201207]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/ira-weiny-intel-com/Lift-memcpy_-to-from-_page-to-core/20201208-070017
base: git://git.infradead.org/users/hch/configfs.git for-next
config: powerpc64-randconfig-r002-20201207 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project a2f922140f5380571fb74179f2bf622b3b925697)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc64 cross compiling tool for clang build
# apt-get install binutils-powerpc64-linux-gnu
# https://github.com/0day-ci/linux/commit/23e6d3f08a315c6e70fde3d63a275c91e1dcb0ee
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review ira-weiny-intel-com/Lift-memcpy_-to-from-_page-to-core/20201208-070017
git checkout 23e6d3f08a315c6e70fde3d63a275c91e1dcb0ee
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>):
<scratch space>:223:1: note: expanded from here
__do_outsw
^
arch/powerpc/include/asm/io.h:545:58: note: expanded from macro '__do_outsw'
#define __do_outsw(p, b, n) writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from arch/powerpc/kernel/asm-offsets.c:23:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:10:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:604:
arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:601:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:225:1: note: expanded from here
__do_outsl
^
arch/powerpc/include/asm/io.h:546:58: note: expanded from macro '__do_outsl'
#define __do_outsl(p, b, n) writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from arch/powerpc/kernel/asm-offsets.c:23:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:22:
In file included from include/linux/writeback.h:14:
In file included from include/linux/blk-cgroup.h:23:
In file included from include/linux/blkdev.h:14:
In file included from include/linux/pagemap.h:11:
include/linux/highmem.h:229:15: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
void *addr = kmap_local_page(page);
^
include/linux/highmem.h:229:15: note: did you mean 'kmap_to_page'?
include/linux/highmem.h:130:28: note: 'kmap_to_page' declared here
static inline struct page *kmap_to_page(void *addr)
^
include/linux/highmem.h:229:8: warning: incompatible integer to pointer conversion initializing 'void *' with an expression of type 'int' [-Wint-conversion]
void *addr = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:231:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(addr);
^
include/linux/highmem.h:282:16: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
void *kaddr = kmap_local_page(page);
^
include/linux/highmem.h:282:8: warning: incompatible integer to pointer conversion initializing 'void *' with an expression of type 'int' [-Wint-conversion]
void *kaddr = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:284:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(kaddr);
^
include/linux/highmem.h:291:16: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
void *kaddr = kmap_local_page(page);
^
include/linux/highmem.h:291:8: warning: incompatible integer to pointer conversion initializing 'void *' with an expression of type 'int' [-Wint-conversion]
void *kaddr = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:301:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(kaddr);
^
include/linux/highmem.h:324:10: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
vfrom = kmap_local_page(from);
^
include/linux/highmem.h:324:8: warning: incompatible integer to pointer conversion assigning to 'char *' from 'int' [-Wint-conversion]
vfrom = kmap_local_page(from);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:325:6: warning: incompatible integer to pointer conversion assigning to 'char *' from 'int' [-Wint-conversion]
vto = kmap_local_page(to);
^ ~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:327:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(vto);
^
include/linux/highmem.h:339:10: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
vfrom = kmap_local_page(from);
^
include/linux/highmem.h:339:8: warning: incompatible integer to pointer conversion assigning to 'char *' from 'int' [-Wint-conversion]
vfrom = kmap_local_page(from);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:340:6: warning: incompatible integer to pointer conversion assigning to 'char *' from 'int' [-Wint-conversion]
vto = kmap_local_page(to);
^ ~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:342:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(vto);
^
In file included from arch/powerpc/kernel/asm-offsets.c:23:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:22:
In file included from include/linux/writeback.h:14:
In file included from include/linux/blk-cgroup.h:23:
In file included from include/linux/blkdev.h:14:
>> include/linux/pagemap.h:1036:14: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
char *dst = kmap_local_page(dst_page);
^
>> include/linux/pagemap.h:1036:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *dst = kmap_local_page(dst_page);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/pagemap.h:1037:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *src = kmap_local_page(src_page);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~
>> include/linux/pagemap.h:1039:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(src);
^
include/linux/pagemap.h:1047:14: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
char *dst = kmap_local_page(dst_page);
^
include/linux/pagemap.h:1047:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *dst = kmap_local_page(dst_page);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/pagemap.h:1048:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *src = kmap_local_page(src_page);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/pagemap.h:1050:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(src);
^
include/linux/pagemap.h:1056:15: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
char *from = kmap_local_page(page);
^
include/linux/pagemap.h:1056:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *from = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/pagemap.h:1058:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(from);
^
include/linux/pagemap.h:1063:13: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
char *to = kmap_local_page(page);
^
include/linux/pagemap.h:1063:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *to = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/pagemap.h:1065:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(to);
^
include/linux/pagemap.h:1070:15: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
char *addr = kmap_local_page(page);
^
include/linux/pagemap.h:1070:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *addr = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 warnings and 20 errors generated.
--
<scratch space>:223:1: note: expanded from here
__do_outsw
^
arch/powerpc/include/asm/io.h:545:58: note: expanded from macro '__do_outsw'
#define __do_outsw(p, b, n) writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from arch/powerpc/kernel/asm-offsets.c:23:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:10:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:604:
arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:601:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:225:1: note: expanded from here
__do_outsl
^
arch/powerpc/include/asm/io.h:546:58: note: expanded from macro '__do_outsl'
#define __do_outsl(p, b, n) writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from arch/powerpc/kernel/asm-offsets.c:23:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:22:
In file included from include/linux/writeback.h:14:
In file included from include/linux/blk-cgroup.h:23:
In file included from include/linux/blkdev.h:14:
In file included from include/linux/pagemap.h:11:
include/linux/highmem.h:229:15: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
void *addr = kmap_local_page(page);
^
include/linux/highmem.h:229:15: note: did you mean 'kmap_to_page'?
include/linux/highmem.h:130:28: note: 'kmap_to_page' declared here
static inline struct page *kmap_to_page(void *addr)
^
include/linux/highmem.h:229:8: warning: incompatible integer to pointer conversion initializing 'void *' with an expression of type 'int' [-Wint-conversion]
void *addr = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:231:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(addr);
^
include/linux/highmem.h:282:16: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
void *kaddr = kmap_local_page(page);
^
include/linux/highmem.h:282:8: warning: incompatible integer to pointer conversion initializing 'void *' with an expression of type 'int' [-Wint-conversion]
void *kaddr = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:284:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(kaddr);
^
include/linux/highmem.h:291:16: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
void *kaddr = kmap_local_page(page);
^
include/linux/highmem.h:291:8: warning: incompatible integer to pointer conversion initializing 'void *' with an expression of type 'int' [-Wint-conversion]
void *kaddr = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:301:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(kaddr);
^
include/linux/highmem.h:324:10: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
vfrom = kmap_local_page(from);
^
include/linux/highmem.h:324:8: warning: incompatible integer to pointer conversion assigning to 'char *' from 'int' [-Wint-conversion]
vfrom = kmap_local_page(from);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:325:6: warning: incompatible integer to pointer conversion assigning to 'char *' from 'int' [-Wint-conversion]
vto = kmap_local_page(to);
^ ~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:327:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(vto);
^
include/linux/highmem.h:339:10: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
vfrom = kmap_local_page(from);
^
include/linux/highmem.h:339:8: warning: incompatible integer to pointer conversion assigning to 'char *' from 'int' [-Wint-conversion]
vfrom = kmap_local_page(from);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:340:6: warning: incompatible integer to pointer conversion assigning to 'char *' from 'int' [-Wint-conversion]
vto = kmap_local_page(to);
^ ~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:342:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(vto);
^
In file included from arch/powerpc/kernel/asm-offsets.c:23:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:22:
In file included from include/linux/writeback.h:14:
In file included from include/linux/blk-cgroup.h:23:
In file included from include/linux/blkdev.h:14:
>> include/linux/pagemap.h:1036:14: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
char *dst = kmap_local_page(dst_page);
^
>> include/linux/pagemap.h:1036:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *dst = kmap_local_page(dst_page);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/pagemap.h:1037:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *src = kmap_local_page(src_page);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~
>> include/linux/pagemap.h:1039:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(src);
^
include/linux/pagemap.h:1047:14: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
char *dst = kmap_local_page(dst_page);
^
include/linux/pagemap.h:1047:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *dst = kmap_local_page(dst_page);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/pagemap.h:1048:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *src = kmap_local_page(src_page);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/pagemap.h:1050:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(src);
^
include/linux/pagemap.h:1056:15: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
char *from = kmap_local_page(page);
^
include/linux/pagemap.h:1056:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *from = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/pagemap.h:1058:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(from);
^
include/linux/pagemap.h:1063:13: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
char *to = kmap_local_page(page);
^
include/linux/pagemap.h:1063:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *to = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/pagemap.h:1065:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(to);
^
include/linux/pagemap.h:1070:15: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
char *addr = kmap_local_page(page);
^
include/linux/pagemap.h:1070:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *addr = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 warnings and 20 errors generated.
make[2]: *** [scripts/Makefile.build:117: arch/powerpc/kernel/asm-offsets.s] Error 1
make[2]: Target '__build' not remade because of errors.
make[1]: *** [Makefile:1200: prepare0] Error 2
make[1]: Target 'prepare' not remade because of errors.
make: *** [Makefile:185: __sub-make] Error 2
make: Target 'prepare' not remade because of errors.
vim +/kmap_local_page +1036 include/linux/pagemap.h
1031
1032 static inline void memcpy_page(struct page *dst_page, size_t dst_off,
1033 struct page *src_page, size_t src_off,
1034 size_t len)
1035 {
> 1036 char *dst = kmap_local_page(dst_page);
1037 char *src = kmap_local_page(src_page);
1038 memcpy(dst + dst_off, src + src_off, len);
> 1039 kunmap_local(src);
1040 kunmap_local(dst);
1041 }
1042
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31963 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
@ 2020-12-08 1:09 ` kernel test robot
0 siblings, 0 replies; 30+ messages in thread
From: kernel test robot @ 2020-12-08 1:09 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 22900 bytes --]
Hi,
I love your patch! Yet something to improve:
[auto build test ERROR on hch-configfs/for-next]
[also build test ERROR on linus/master v5.10-rc7]
[cannot apply to hnaz-linux-mm/master next-20201207]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/ira-weiny-intel-com/Lift-memcpy_-to-from-_page-to-core/20201208-070017
base: git://git.infradead.org/users/hch/configfs.git for-next
config: powerpc64-randconfig-r002-20201207 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project a2f922140f5380571fb74179f2bf622b3b925697)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc64 cross compiling tool for clang build
# apt-get install binutils-powerpc64-linux-gnu
# https://github.com/0day-ci/linux/commit/23e6d3f08a315c6e70fde3d63a275c91e1dcb0ee
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review ira-weiny-intel-com/Lift-memcpy_-to-from-_page-to-core/20201208-070017
git checkout 23e6d3f08a315c6e70fde3d63a275c91e1dcb0ee
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>):
<scratch space>:223:1: note: expanded from here
__do_outsw
^
arch/powerpc/include/asm/io.h:545:58: note: expanded from macro '__do_outsw'
#define __do_outsw(p, b, n) writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from arch/powerpc/kernel/asm-offsets.c:23:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:10:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:604:
arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:601:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:225:1: note: expanded from here
__do_outsl
^
arch/powerpc/include/asm/io.h:546:58: note: expanded from macro '__do_outsl'
#define __do_outsl(p, b, n) writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from arch/powerpc/kernel/asm-offsets.c:23:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:22:
In file included from include/linux/writeback.h:14:
In file included from include/linux/blk-cgroup.h:23:
In file included from include/linux/blkdev.h:14:
In file included from include/linux/pagemap.h:11:
include/linux/highmem.h:229:15: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
void *addr = kmap_local_page(page);
^
include/linux/highmem.h:229:15: note: did you mean 'kmap_to_page'?
include/linux/highmem.h:130:28: note: 'kmap_to_page' declared here
static inline struct page *kmap_to_page(void *addr)
^
include/linux/highmem.h:229:8: warning: incompatible integer to pointer conversion initializing 'void *' with an expression of type 'int' [-Wint-conversion]
void *addr = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:231:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(addr);
^
include/linux/highmem.h:282:16: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
void *kaddr = kmap_local_page(page);
^
include/linux/highmem.h:282:8: warning: incompatible integer to pointer conversion initializing 'void *' with an expression of type 'int' [-Wint-conversion]
void *kaddr = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:284:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(kaddr);
^
include/linux/highmem.h:291:16: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
void *kaddr = kmap_local_page(page);
^
include/linux/highmem.h:291:8: warning: incompatible integer to pointer conversion initializing 'void *' with an expression of type 'int' [-Wint-conversion]
void *kaddr = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:301:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(kaddr);
^
include/linux/highmem.h:324:10: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
vfrom = kmap_local_page(from);
^
include/linux/highmem.h:324:8: warning: incompatible integer to pointer conversion assigning to 'char *' from 'int' [-Wint-conversion]
vfrom = kmap_local_page(from);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:325:6: warning: incompatible integer to pointer conversion assigning to 'char *' from 'int' [-Wint-conversion]
vto = kmap_local_page(to);
^ ~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:327:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(vto);
^
include/linux/highmem.h:339:10: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
vfrom = kmap_local_page(from);
^
include/linux/highmem.h:339:8: warning: incompatible integer to pointer conversion assigning to 'char *' from 'int' [-Wint-conversion]
vfrom = kmap_local_page(from);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:340:6: warning: incompatible integer to pointer conversion assigning to 'char *' from 'int' [-Wint-conversion]
vto = kmap_local_page(to);
^ ~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:342:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(vto);
^
In file included from arch/powerpc/kernel/asm-offsets.c:23:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:22:
In file included from include/linux/writeback.h:14:
In file included from include/linux/blk-cgroup.h:23:
In file included from include/linux/blkdev.h:14:
>> include/linux/pagemap.h:1036:14: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
char *dst = kmap_local_page(dst_page);
^
>> include/linux/pagemap.h:1036:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *dst = kmap_local_page(dst_page);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/pagemap.h:1037:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *src = kmap_local_page(src_page);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~
>> include/linux/pagemap.h:1039:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(src);
^
include/linux/pagemap.h:1047:14: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
char *dst = kmap_local_page(dst_page);
^
include/linux/pagemap.h:1047:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *dst = kmap_local_page(dst_page);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/pagemap.h:1048:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *src = kmap_local_page(src_page);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/pagemap.h:1050:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(src);
^
include/linux/pagemap.h:1056:15: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
char *from = kmap_local_page(page);
^
include/linux/pagemap.h:1056:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *from = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/pagemap.h:1058:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(from);
^
include/linux/pagemap.h:1063:13: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
char *to = kmap_local_page(page);
^
include/linux/pagemap.h:1063:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *to = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/pagemap.h:1065:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(to);
^
include/linux/pagemap.h:1070:15: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
char *addr = kmap_local_page(page);
^
include/linux/pagemap.h:1070:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *addr = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 warnings and 20 errors generated.
--
<scratch space>:223:1: note: expanded from here
__do_outsw
^
arch/powerpc/include/asm/io.h:545:58: note: expanded from macro '__do_outsw'
#define __do_outsw(p, b, n) writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from arch/powerpc/kernel/asm-offsets.c:23:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:10:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:604:
arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:601:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:225:1: note: expanded from here
__do_outsl
^
arch/powerpc/include/asm/io.h:546:58: note: expanded from macro '__do_outsl'
#define __do_outsl(p, b, n) writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from arch/powerpc/kernel/asm-offsets.c:23:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:22:
In file included from include/linux/writeback.h:14:
In file included from include/linux/blk-cgroup.h:23:
In file included from include/linux/blkdev.h:14:
In file included from include/linux/pagemap.h:11:
include/linux/highmem.h:229:15: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
void *addr = kmap_local_page(page);
^
include/linux/highmem.h:229:15: note: did you mean 'kmap_to_page'?
include/linux/highmem.h:130:28: note: 'kmap_to_page' declared here
static inline struct page *kmap_to_page(void *addr)
^
include/linux/highmem.h:229:8: warning: incompatible integer to pointer conversion initializing 'void *' with an expression of type 'int' [-Wint-conversion]
void *addr = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:231:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(addr);
^
include/linux/highmem.h:282:16: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
void *kaddr = kmap_local_page(page);
^
include/linux/highmem.h:282:8: warning: incompatible integer to pointer conversion initializing 'void *' with an expression of type 'int' [-Wint-conversion]
void *kaddr = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:284:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(kaddr);
^
include/linux/highmem.h:291:16: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
void *kaddr = kmap_local_page(page);
^
include/linux/highmem.h:291:8: warning: incompatible integer to pointer conversion initializing 'void *' with an expression of type 'int' [-Wint-conversion]
void *kaddr = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:301:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(kaddr);
^
include/linux/highmem.h:324:10: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
vfrom = kmap_local_page(from);
^
include/linux/highmem.h:324:8: warning: incompatible integer to pointer conversion assigning to 'char *' from 'int' [-Wint-conversion]
vfrom = kmap_local_page(from);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:325:6: warning: incompatible integer to pointer conversion assigning to 'char *' from 'int' [-Wint-conversion]
vto = kmap_local_page(to);
^ ~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:327:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(vto);
^
include/linux/highmem.h:339:10: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
vfrom = kmap_local_page(from);
^
include/linux/highmem.h:339:8: warning: incompatible integer to pointer conversion assigning to 'char *' from 'int' [-Wint-conversion]
vfrom = kmap_local_page(from);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:340:6: warning: incompatible integer to pointer conversion assigning to 'char *' from 'int' [-Wint-conversion]
vto = kmap_local_page(to);
^ ~~~~~~~~~~~~~~~~~~~
include/linux/highmem.h:342:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(vto);
^
In file included from arch/powerpc/kernel/asm-offsets.c:23:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:22:
In file included from include/linux/writeback.h:14:
In file included from include/linux/blk-cgroup.h:23:
In file included from include/linux/blkdev.h:14:
>> include/linux/pagemap.h:1036:14: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
char *dst = kmap_local_page(dst_page);
^
>> include/linux/pagemap.h:1036:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *dst = kmap_local_page(dst_page);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/pagemap.h:1037:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *src = kmap_local_page(src_page);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~
>> include/linux/pagemap.h:1039:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(src);
^
include/linux/pagemap.h:1047:14: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
char *dst = kmap_local_page(dst_page);
^
include/linux/pagemap.h:1047:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *dst = kmap_local_page(dst_page);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/pagemap.h:1048:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *src = kmap_local_page(src_page);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/pagemap.h:1050:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(src);
^
include/linux/pagemap.h:1056:15: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
char *from = kmap_local_page(page);
^
include/linux/pagemap.h:1056:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *from = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/pagemap.h:1058:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(from);
^
include/linux/pagemap.h:1063:13: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
char *to = kmap_local_page(page);
^
include/linux/pagemap.h:1063:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *to = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
include/linux/pagemap.h:1065:2: error: implicit declaration of function 'kunmap_local' [-Werror,-Wimplicit-function-declaration]
kunmap_local(to);
^
include/linux/pagemap.h:1070:15: error: implicit declaration of function 'kmap_local_page' [-Werror,-Wimplicit-function-declaration]
char *addr = kmap_local_page(page);
^
include/linux/pagemap.h:1070:8: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
char *addr = kmap_local_page(page);
^ ~~~~~~~~~~~~~~~~~~~~~
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 warnings and 20 errors generated.
make[2]: *** [scripts/Makefile.build:117: arch/powerpc/kernel/asm-offsets.s] Error 1
make[2]: Target '__build' not remade because of errors.
make[1]: *** [Makefile:1200: prepare0] Error 2
make[1]: Target 'prepare' not remade because of errors.
make: *** [Makefile:185: __sub-make] Error 2
make: Target 'prepare' not remade because of errors.
vim +/kmap_local_page +1036 include/linux/pagemap.h
1031
1032 static inline void memcpy_page(struct page *dst_page, size_t dst_off,
1033 struct page *src_page, size_t src_off,
1034 size_t len)
1035 {
> 1036 char *dst = kmap_local_page(dst_page);
1037 char *src = kmap_local_page(src_page);
1038 memcpy(dst + dst_off, src + src_off, len);
> 1039 kunmap_local(src);
1040 kunmap_local(dst);
1041 }
1042
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 31963 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-07 22:57 ` [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core ira.weiny
` (2 preceding siblings ...)
2020-12-08 1:09 ` kernel test robot
@ 2020-12-08 12:23 ` Matthew Wilcox
2020-12-08 16:38 ` Ira Weiny
3 siblings, 1 reply; 30+ messages in thread
From: Matthew Wilcox @ 2020-12-08 12:23 UTC (permalink / raw)
To: ira.weiny
Cc: Thomas Gleixner, Andrew Morton, Dave Hansen, Christoph Hellwig,
Dan Williams, Al Viro, Eric Biggers, Joonas Lahtinen,
linux-kernel, linux-fsdevel
On Mon, Dec 07, 2020 at 02:57:03PM -0800, ira.weiny@intel.com wrote:
> Placing these functions in 'highmem.h' is suboptimal especially with the
> changes being proposed in the functionality of kmap. From a caller
> perspective including/using 'highmem.h' implies that the functions
> defined in that header are only required when highmem is in use which is
> increasingly not the case with modern processors. Some headers like
> mm.h or string.h seem ok but don't really portray the functionality
> well. 'pagemap.h', on the other hand, makes sense and is already
> included in many of the places we want to convert.
pagemap.h is for the page cache. It's not for "random page
functionality". Yes, I know it's badly named. No, I don't want to
rename it. These helpers should go in highmem.h along with zero_user().
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-08 12:23 ` Matthew Wilcox
@ 2020-12-08 16:38 ` Ira Weiny
2020-12-08 16:40 ` Matthew Wilcox
0 siblings, 1 reply; 30+ messages in thread
From: Ira Weiny @ 2020-12-08 16:38 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Thomas Gleixner, Andrew Morton, Dave Hansen, Christoph Hellwig,
Dan Williams, Al Viro, Eric Biggers, Joonas Lahtinen,
linux-kernel, linux-fsdevel
On Tue, Dec 08, 2020 at 12:23:16PM +0000, Matthew Wilcox wrote:
> On Mon, Dec 07, 2020 at 02:57:03PM -0800, ira.weiny@intel.com wrote:
> > Placing these functions in 'highmem.h' is suboptimal especially with the
> > changes being proposed in the functionality of kmap. From a caller
> > perspective including/using 'highmem.h' implies that the functions
> > defined in that header are only required when highmem is in use which is
> > increasingly not the case with modern processors. Some headers like
> > mm.h or string.h seem ok but don't really portray the functionality
> > well. 'pagemap.h', on the other hand, makes sense and is already
> > included in many of the places we want to convert.
>
> pagemap.h is for the page cache. It's not for "random page
> functionality". Yes, I know it's badly named. No, I don't want to
> rename it. These helpers should go in highmem.h along with zero_user().
I could have sworn you suggested pagemap.h. But I can't find the evidence on
lore. :-/ hehehe...
In the end the code does not care. I have a distaste for highmem.h because it
is no longer for 'high memory'. And I think a number of driver writers who are
targeting 64bit platforms just don't care any more. So as we head toward
memory not being mapped by the kernel for other reasons I think highmem needs
to be 'rebranded' if not renamed.
Ira
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core
2020-12-08 16:38 ` Ira Weiny
@ 2020-12-08 16:40 ` Matthew Wilcox
0 siblings, 0 replies; 30+ messages in thread
From: Matthew Wilcox @ 2020-12-08 16:40 UTC (permalink / raw)
To: Ira Weiny
Cc: Thomas Gleixner, Andrew Morton, Dave Hansen, Christoph Hellwig,
Dan Williams, Al Viro, Eric Biggers, Joonas Lahtinen,
linux-kernel, linux-fsdevel
On Tue, Dec 08, 2020 at 08:38:14AM -0800, Ira Weiny wrote:
> On Tue, Dec 08, 2020 at 12:23:16PM +0000, Matthew Wilcox wrote:
> > On Mon, Dec 07, 2020 at 02:57:03PM -0800, ira.weiny@intel.com wrote:
> > > Placing these functions in 'highmem.h' is suboptimal especially with the
> > > changes being proposed in the functionality of kmap. From a caller
> > > perspective including/using 'highmem.h' implies that the functions
> > > defined in that header are only required when highmem is in use which is
> > > increasingly not the case with modern processors. Some headers like
> > > mm.h or string.h seem ok but don't really portray the functionality
> > > well. 'pagemap.h', on the other hand, makes sense and is already
> > > included in many of the places we want to convert.
> >
> > pagemap.h is for the page cache. It's not for "random page
> > functionality". Yes, I know it's badly named. No, I don't want to
> > rename it. These helpers should go in highmem.h along with zero_user().
>
> I could have sworn you suggested pagemap.h. But I can't find the evidence on
> lore. :-/ hehehe...
>
> In the end the code does not care. I have a distaste for highmem.h because it
> is no longer for 'high memory'. And I think a number of driver writers who are
> targeting 64bit platforms just don't care any more. So as we head toward
> memory not being mapped by the kernel for other reasons I think highmem needs
> to be 'rebranded' if not renamed.
Rename highmem.h to kmap.h?
^ permalink raw reply [flat|nested] 30+ messages in thread