Re: [PATCH -mm -V2] mm, swap: Fix false error message in __swp_swapcount()

[Michal Hocko]

On Tue 31-10-17 16:37:10, Minchan Kim wrote:
> On Tue, Oct 31, 2017 at 08:31:07AM +0100, Michal Hocko wrote:
> > On Tue 31-10-17 11:17:02, Minchan Kim wrote:
> > > On Mon, Oct 30, 2017 at 09:02:30AM +0100, Michal Hocko wrote:
> > > > On Mon 30-10-17 08:57:13, Minchan Kim wrote:
> > > > [...]
> > > > > Although it's better than old, we can make it simple, still.
> > > > > 
> > > > > diff --git a/include/linux/swapops.h b/include/linux/swapops.h
> > > > > index 291c4b534658..f50d5a48f03a 100644
> > > > > --- a/include/linux/swapops.h
> > > > > +++ b/include/linux/swapops.h
> > > > > @@ -41,6 +41,13 @@ static inline unsigned swp_type(swp_entry_t entry)
> > > > >   return (entry.val >> SWP_TYPE_SHIFT(entry));
> > > > >  }
> > > > >  
> > > > > +extern struct swap_info_struct *swap_info[];
> > > > > +
> > > > > +static inline struct swap_info_struct *swp_si(swp_entry_t entry)
> > > > > +{
> > > > > + return swap_info[swp_type(entry)];
> > > > > +}
> > > > > +
> > > > >  /*
> > > > >   * Extract the `offset' field from a swp_entry_t.  The swp_entry_t 
> > > > > is in
> > > > >   * arch-independent format
> > > > > diff --git a/mm/swap_state.c b/mm/swap_state.c
> > > > > index 378262d3a197..a0fe2d54ad09 100644
> > > > > --- a/mm/swap_state.c
> > > > > +++ b/mm/swap_state.c
> > > > > @@ -554,6 +554,7 @@ struct page *swapin_readahead(swp_entry_t entry, 
> > > > > gfp_t gfp_mask,
> > > > >   struct vm_area_struct *vma, unsigned long addr)
> > > > >  {
> > > > >   struct page *page;
> > > > > + struct swap_info_struct *si = swp_si(entry);
> > > > 
> > > > Aren't you accessing beyond the array here?
> > > 
> > > I couldn't understand what you intend. Could you explain what case does 
> > > it accesses
> > > beyond the arrary?
> > 
> > what if swp_type > nr_swapfiles
> 
> Logicall, it shouldn't be happen but it's a bug once it happens.
> Such a bug can happen everywhere.
> Why do you want to take care in this logic?

You are right. It was my misunderstanding of the patch. I though the
readahead swap entries could overflow swp_type as well as the offset.
My bad.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH -mm -V2] mm, swap: Fix false error message in __swp_swapcount()

[Michal Hocko]

On Tue 31-10-17 16:37:10, Minchan Kim wrote:
> On Tue, Oct 31, 2017 at 08:31:07AM +0100, Michal Hocko wrote:
> > On Tue 31-10-17 11:17:02, Minchan Kim wrote:
> > > On Mon, Oct 30, 2017 at 09:02:30AM +0100, Michal Hocko wrote:
> > > > On Mon 30-10-17 08:57:13, Minchan Kim wrote:
> > > > [...]
> > > > > Although it's better than old, we can make it simple, still.
> > > > > 
> > > > > diff --git a/include/linux/swapops.h b/include/linux/swapops.h
> > > > > index 291c4b534658..f50d5a48f03a 100644
> > > > > --- a/include/linux/swapops.h
> > > > > +++ b/include/linux/swapops.h
> > > > > @@ -41,6 +41,13 @@ static inline unsigned swp_type(swp_entry_t entry)
> > > > >   return (entry.val >> SWP_TYPE_SHIFT(entry));
> > > > >  }
> > > > >  
> > > > > +extern struct swap_info_struct *swap_info[];
> > > > > +
> > > > > +static inline struct swap_info_struct *swp_si(swp_entry_t entry)
> > > > > +{
> > > > > + return swap_info[swp_type(entry)];
> > > > > +}
> > > > > +
> > > > >  /*
> > > > >   * Extract the `offset' field from a swp_entry_t.  The swp_entry_t 
> > > > > is in
> > > > >   * arch-independent format
> > > > > diff --git a/mm/swap_state.c b/mm/swap_state.c
> > > > > index 378262d3a197..a0fe2d54ad09 100644
> > > > > --- a/mm/swap_state.c
> > > > > +++ b/mm/swap_state.c
> > > > > @@ -554,6 +554,7 @@ struct page *swapin_readahead(swp_entry_t entry, 
> > > > > gfp_t gfp_mask,
> > > > >   struct vm_area_struct *vma, unsigned long addr)
> > > > >  {
> > > > >   struct page *page;
> > > > > + struct swap_info_struct *si = swp_si(entry);
> > > > 
> > > > Aren't you accessing beyond the array here?
> > > 
> > > I couldn't understand what you intend. Could you explain what case does 
> > > it accesses
> > > beyond the arrary?
> > 
> > what if swp_type > nr_swapfiles
> 
> Logicall, it shouldn't be happen but it's a bug once it happens.
> Such a bug can happen everywhere.
> Why do you want to take care in this logic?

You are right. It was my misunderstanding of the patch. I though the
readahead swap entries could overflow swp_type as well as the offset.
My bad.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH -mm -V2] mm, swap: Fix false error message in __swp_swapcount()

[Minchan Kim]

On Tue, Oct 31, 2017 at 08:31:07AM +0100, Michal Hocko wrote:
> On Tue 31-10-17 11:17:02, Minchan Kim wrote:
> > On Mon, Oct 30, 2017 at 09:02:30AM +0100, Michal Hocko wrote:
> > > On Mon 30-10-17 08:57:13, Minchan Kim wrote:
> > > [...]
> > > > Although it's better than old, we can make it simple, still.
> > > > 
> > > > diff --git a/include/linux/swapops.h b/include/linux/swapops.h
> > > > index 291c4b534658..f50d5a48f03a 100644
> > > > --- a/include/linux/swapops.h
> > > > +++ b/include/linux/swapops.h
> > > > @@ -41,6 +41,13 @@ static inline unsigned swp_type(swp_entry_t entry)
> > > > return (entry.val >> SWP_TYPE_SHIFT(entry));
> > > >  }
> > > >  
> > > > +extern struct swap_info_struct *swap_info[];
> > > > +
> > > > +static inline struct swap_info_struct *swp_si(swp_entry_t entry)
> > > > +{
> > > > +   return swap_info[swp_type(entry)];
> > > > +}
> > > > +
> > > >  /*
> > > >   * Extract the `offset' field from a swp_entry_t.  The swp_entry_t is 
> > > > in
> > > >   * arch-independent format
> > > > diff --git a/mm/swap_state.c b/mm/swap_state.c
> > > > index 378262d3a197..a0fe2d54ad09 100644
> > > > --- a/mm/swap_state.c
> > > > +++ b/mm/swap_state.c
> > > > @@ -554,6 +554,7 @@ struct page *swapin_readahead(swp_entry_t entry, 
> > > > gfp_t gfp_mask,
> > > > struct vm_area_struct *vma, unsigned long addr)
> > > >  {
> > > > struct page *page;
> > > > +   struct swap_info_struct *si = swp_si(entry);
> > > 
> > > Aren't you accessing beyond the array here?
> > 
> > I couldn't understand what you intend. Could you explain what case does it 
> > accesses
> > beyond the arrary?
> 
> what if swp_type > nr_swapfiles

Logicall, it shouldn't be happen but it's a bug once it happens.
Such a bug can happen everywhere.
Why do you want to take care in this logic?

Re: [PATCH -mm -V2] mm, swap: Fix false error message in __swp_swapcount()

[Minchan Kim]

On Tue, Oct 31, 2017 at 08:31:07AM +0100, Michal Hocko wrote:
> On Tue 31-10-17 11:17:02, Minchan Kim wrote:
> > On Mon, Oct 30, 2017 at 09:02:30AM +0100, Michal Hocko wrote:
> > > On Mon 30-10-17 08:57:13, Minchan Kim wrote:
> > > [...]
> > > > Although it's better than old, we can make it simple, still.
> > > > 
> > > > diff --git a/include/linux/swapops.h b/include/linux/swapops.h
> > > > index 291c4b534658..f50d5a48f03a 100644
> > > > --- a/include/linux/swapops.h
> > > > +++ b/include/linux/swapops.h
> > > > @@ -41,6 +41,13 @@ static inline unsigned swp_type(swp_entry_t entry)
> > > > return (entry.val >> SWP_TYPE_SHIFT(entry));
> > > >  }
> > > >  
> > > > +extern struct swap_info_struct *swap_info[];
> > > > +
> > > > +static inline struct swap_info_struct *swp_si(swp_entry_t entry)
> > > > +{
> > > > +   return swap_info[swp_type(entry)];
> > > > +}
> > > > +
> > > >  /*
> > > >   * Extract the `offset' field from a swp_entry_t.  The swp_entry_t is 
> > > > in
> > > >   * arch-independent format
> > > > diff --git a/mm/swap_state.c b/mm/swap_state.c
> > > > index 378262d3a197..a0fe2d54ad09 100644
> > > > --- a/mm/swap_state.c
> > > > +++ b/mm/swap_state.c
> > > > @@ -554,6 +554,7 @@ struct page *swapin_readahead(swp_entry_t entry, 
> > > > gfp_t gfp_mask,
> > > > struct vm_area_struct *vma, unsigned long addr)
> > > >  {
> > > > struct page *page;
> > > > +   struct swap_info_struct *si = swp_si(entry);
> > > 
> > > Aren't you accessing beyond the array here?
> > 
> > I couldn't understand what you intend. Could you explain what case does it 
> > accesses
> > beyond the arrary?
> 
> what if swp_type > nr_swapfiles

Logicall, it shouldn't be happen but it's a bug once it happens.
Such a bug can happen everywhere.
Why do you want to take care in this logic?

Re: [PATCH -mm -V2] mm, swap: Fix false error message in __swp_swapcount()

[Michal Hocko]

On Tue 31-10-17 11:17:02, Minchan Kim wrote:
> On Mon, Oct 30, 2017 at 09:02:30AM +0100, Michal Hocko wrote:
> > On Mon 30-10-17 08:57:13, Minchan Kim wrote:
> > [...]
> > > Although it's better than old, we can make it simple, still.
> > > 
> > > diff --git a/include/linux/swapops.h b/include/linux/swapops.h
> > > index 291c4b534658..f50d5a48f03a 100644
> > > --- a/include/linux/swapops.h
> > > +++ b/include/linux/swapops.h
> > > @@ -41,6 +41,13 @@ static inline unsigned swp_type(swp_entry_t entry)
> > >   return (entry.val >> SWP_TYPE_SHIFT(entry));
> > >  }
> > >  
> > > +extern struct swap_info_struct *swap_info[];
> > > +
> > > +static inline struct swap_info_struct *swp_si(swp_entry_t entry)
> > > +{
> > > + return swap_info[swp_type(entry)];
> > > +}
> > > +
> > >  /*
> > >   * Extract the `offset' field from a swp_entry_t.  The swp_entry_t is in
> > >   * arch-independent format
> > > diff --git a/mm/swap_state.c b/mm/swap_state.c
> > > index 378262d3a197..a0fe2d54ad09 100644
> > > --- a/mm/swap_state.c
> > > +++ b/mm/swap_state.c
> > > @@ -554,6 +554,7 @@ struct page *swapin_readahead(swp_entry_t entry, 
> > > gfp_t gfp_mask,
> > >   struct vm_area_struct *vma, unsigned long addr)
> > >  {
> > >   struct page *page;
> > > + struct swap_info_struct *si = swp_si(entry);
> > 
> > Aren't you accessing beyond the array here?
> 
> I couldn't understand what you intend. Could you explain what case does it 
> accesses
> beyond the arrary?

what if swp_type > nr_swapfiles
-- 
Michal Hocko
SUSE Labs

Re: [PATCH -mm -V2] mm, swap: Fix false error message in __swp_swapcount()

[Michal Hocko]

On Tue 31-10-17 11:17:02, Minchan Kim wrote:
> On Mon, Oct 30, 2017 at 09:02:30AM +0100, Michal Hocko wrote:
> > On Mon 30-10-17 08:57:13, Minchan Kim wrote:
> > [...]
> > > Although it's better than old, we can make it simple, still.
> > > 
> > > diff --git a/include/linux/swapops.h b/include/linux/swapops.h
> > > index 291c4b534658..f50d5a48f03a 100644
> > > --- a/include/linux/swapops.h
> > > +++ b/include/linux/swapops.h
> > > @@ -41,6 +41,13 @@ static inline unsigned swp_type(swp_entry_t entry)
> > >   return (entry.val >> SWP_TYPE_SHIFT(entry));
> > >  }
> > >  
> > > +extern struct swap_info_struct *swap_info[];
> > > +
> > > +static inline struct swap_info_struct *swp_si(swp_entry_t entry)
> > > +{
> > > + return swap_info[swp_type(entry)];
> > > +}
> > > +
> > >  /*
> > >   * Extract the `offset' field from a swp_entry_t.  The swp_entry_t is in
> > >   * arch-independent format
> > > diff --git a/mm/swap_state.c b/mm/swap_state.c
> > > index 378262d3a197..a0fe2d54ad09 100644
> > > --- a/mm/swap_state.c
> > > +++ b/mm/swap_state.c
> > > @@ -554,6 +554,7 @@ struct page *swapin_readahead(swp_entry_t entry, 
> > > gfp_t gfp_mask,
> > >   struct vm_area_struct *vma, unsigned long addr)
> > >  {
> > >   struct page *page;
> > > + struct swap_info_struct *si = swp_si(entry);
> > 
> > Aren't you accessing beyond the array here?
> 
> I couldn't understand what you intend. Could you explain what case does it 
> accesses
> beyond the arrary?

what if swp_type > nr_swapfiles
-- 
Michal Hocko
SUSE Labs

Re: [PATCH -mm -V2] mm, swap: Fix false error message in __swp_swapcount()

[Huang, Ying]

Minchan Kim  writes:

> Hi Huang,
>
> On Tue, Oct 31, 2017 at 01:32:32PM +0800, Huang, Ying wrote:
>> Hi, Minchan,
>> 
>> Minchan Kim  writes:
>> 
>> > Hi Huang,
>> >
>> > On Fri, Oct 27, 2017 at 01:53:27PM +0800, Huang, Ying wrote:
>> >> From: Huang Ying 
>> >> 
>> >> When a page fault occurs for a swap entry, the physical swap readahead
>> >> (not the VMA base swap readahead) may readahead several swap entries
>> >> after the fault swap entry.  The readahead algorithm calculates some
>> >> of the swap entries to readahead via increasing the offset of the
>> >> fault swap entry without checking whether they are beyond the end of
>> >> the swap device and it relys on the __swp_swapcount() and
>> >> swapcache_prepare() to check it.  Although __swp_swapcount() checks
>> >> for the swap entry passed in, it will complain with the error message
>> >> as follow for the expected invalid swap entry.  This may make the end
>> >> users confused.
>> >> 
>> >>   swap_info_get: Bad swap offset entry 0200f8a7
>> >> 
>> >> To fix the false error message, the swap entry checking is added in
>> >> swap readahead to avoid to pass the out-bound swap entries and the
>> >> swap entry reserved for the swap header to __swp_swapcount() and
>> >> swapcache_prepare().
>> >> 
>> >> Cc: Tim Chen 
>> >> Cc: Minchan Kim 
>> >> Cc: Michal Hocko 
>> >> Cc:  # 4.11-4.13
>> >> Reported-by: Christian Kujau 
>> >> Fixes: e8c26ab60598 ("mm/swap: skip readahead for unreferenced swap 
>> >> slots")
>> >> Signed-off-by: "Huang, Ying" 
>> >> ---
>> >>  include/linux/swap.h |  1 +
>> >>  mm/swap_state.c  |  6 --
>> >>  mm/swapfile.c| 21 +
>> >>  3 files changed, 26 insertions(+), 2 deletions(-)
>> >> 
>> >> diff --git a/include/linux/swap.h b/include/linux/swap.h
>> >> index 84255b3da7c1..43b4b821c805 100644
>> >> --- a/include/linux/swap.h
>> >> +++ b/include/linux/swap.h
>> >> @@ -476,6 +476,7 @@ extern int page_swapcount(struct page *);
>> >>  extern int __swap_count(struct swap_info_struct *si, swp_entry_t entry);
>> >>  extern int __swp_swapcount(swp_entry_t entry);
>> >>  extern int swp_swapcount(swp_entry_t entry);
>> >> +extern bool swap_entry_check(swp_entry_t entry);
>> >>  extern struct swap_info_struct *page_swap_info(struct page *);
>> >>  extern struct swap_info_struct *swp_swap_info(swp_entry_t entry);
>> >>  extern bool reuse_swap_page(struct page *, int *);
>> >> diff --git a/mm/swap_state.c b/mm/swap_state.c
>> >> index 6c017ced11e6..7dd70e77058d 100644
>> >> --- a/mm/swap_state.c
>> >> +++ b/mm/swap_state.c
>> >> @@ -569,11 +569,13 @@ struct page *swapin_readahead(swp_entry_t entry, 
>> >> gfp_t gfp_mask,
>> >>   /* Read a page_cluster sized and aligned cluster around offset. */
>> >>   start_offset = offset & ~mask;
>> >>   end_offset = offset | mask;
>> >> - if (!start_offset)  /* First page is swap header. */
>> >> - start_offset++;
>> >>  
>> >>   blk_start_plug();
>> >>   for (offset = start_offset; offset <= end_offset ; offset++) {
>> >> + swp_entry_t ent = swp_entry(swp_type(entry), offset);
>> >> +
>> >> + if (!swap_entry_check(ent))
>> >> + continue;
>> >>   /* Ok, do the async read-ahead now */
>> >>   page = __read_swap_cache_async(
>> >>   swp_entry(swp_type(entry), offset),
>> >> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> >> index 3074b02eaa09..b04cec29c234 100644
>> >> --- a/mm/swapfile.c
>> >> +++ b/mm/swapfile.c
>> >> @@ -1107,6 +1107,27 @@ static struct swap_info_struct 
>> >> *swap_info_get_cont(swp_entry_t entry,
>> >>   return p;
>> >>  }
>> >>  
>> >> +bool swap_entry_check(swp_entry_t entry)
>> >> +{
>> >> + struct swap_info_struct *p;
>> >> + unsigned long offset, type;
>> >> +
>> >> + type = swp_type(entry);
>> >> + if (type >= nr_swapfiles)
>> >> + goto bad_file;
>> >> + p = swap_info[type];
>> >> + offset = swp_offset(entry);
>> >> + if (unlikely(!offset || offset >= p->max))
>> >> + goto out;
>> >> +
>> >> + return true;
>> >> +
>> >> +bad_file:
>> >> + pr_err("%s: %s%08lx\n", __func__, Bad_file, entry.val);
>> >> +out:
>> >> + return false;
>> >> +}
>> >> +
>> >>  static unsigned char __swap_entry_free(struct swap_info_struct *p,
>> >>  swp_entry_t entry, unsigned char usage)
>> >>  {
>> >> -- 
>> >> 2.14.2
>> >
>> > Although it's better than old, we can make it simple, still.
>> >
>> > diff --git a/include/linux/swapops.h b/include/linux/swapops.h
>> > index 291c4b534658..f50d5a48f03a 100644
>> > --- a/include/linux/swapops.h
>> > +++ b/include/linux/swapops.h
>> > @@ -41,6 +41,13 @@ static inline unsigned swp_type(swp_entry_t entry)
>> >return (entry.val >> SWP_TYPE_SHIFT(entry));
>> >  }
>> >  
>> > +extern struct swap_info_struct 

Re: [PATCH -mm -V2] mm, swap: Fix false error message in __swp_swapcount()

[Huang, Ying]

Minchan Kim  writes:

> Hi Huang,
>
> On Tue, Oct 31, 2017 at 01:32:32PM +0800, Huang, Ying wrote:
>> Hi, Minchan,
>> 
>> Minchan Kim  writes:
>> 
>> > Hi Huang,
>> >
>> > On Fri, Oct 27, 2017 at 01:53:27PM +0800, Huang, Ying wrote:
>> >> From: Huang Ying 
>> >> 
>> >> When a page fault occurs for a swap entry, the physical swap readahead
>> >> (not the VMA base swap readahead) may readahead several swap entries
>> >> after the fault swap entry.  The readahead algorithm calculates some
>> >> of the swap entries to readahead via increasing the offset of the
>> >> fault swap entry without checking whether they are beyond the end of
>> >> the swap device and it relys on the __swp_swapcount() and
>> >> swapcache_prepare() to check it.  Although __swp_swapcount() checks
>> >> for the swap entry passed in, it will complain with the error message
>> >> as follow for the expected invalid swap entry.  This may make the end
>> >> users confused.
>> >> 
>> >>   swap_info_get: Bad swap offset entry 0200f8a7
>> >> 
>> >> To fix the false error message, the swap entry checking is added in
>> >> swap readahead to avoid to pass the out-bound swap entries and the
>> >> swap entry reserved for the swap header to __swp_swapcount() and
>> >> swapcache_prepare().
>> >> 
>> >> Cc: Tim Chen 
>> >> Cc: Minchan Kim 
>> >> Cc: Michal Hocko 
>> >> Cc:  # 4.11-4.13
>> >> Reported-by: Christian Kujau 
>> >> Fixes: e8c26ab60598 ("mm/swap: skip readahead for unreferenced swap 
>> >> slots")
>> >> Signed-off-by: "Huang, Ying" 
>> >> ---
>> >>  include/linux/swap.h |  1 +
>> >>  mm/swap_state.c  |  6 --
>> >>  mm/swapfile.c| 21 +
>> >>  3 files changed, 26 insertions(+), 2 deletions(-)
>> >> 
>> >> diff --git a/include/linux/swap.h b/include/linux/swap.h
>> >> index 84255b3da7c1..43b4b821c805 100644
>> >> --- a/include/linux/swap.h
>> >> +++ b/include/linux/swap.h
>> >> @@ -476,6 +476,7 @@ extern int page_swapcount(struct page *);
>> >>  extern int __swap_count(struct swap_info_struct *si, swp_entry_t entry);
>> >>  extern int __swp_swapcount(swp_entry_t entry);
>> >>  extern int swp_swapcount(swp_entry_t entry);
>> >> +extern bool swap_entry_check(swp_entry_t entry);
>> >>  extern struct swap_info_struct *page_swap_info(struct page *);
>> >>  extern struct swap_info_struct *swp_swap_info(swp_entry_t entry);
>> >>  extern bool reuse_swap_page(struct page *, int *);
>> >> diff --git a/mm/swap_state.c b/mm/swap_state.c
>> >> index 6c017ced11e6..7dd70e77058d 100644
>> >> --- a/mm/swap_state.c
>> >> +++ b/mm/swap_state.c
>> >> @@ -569,11 +569,13 @@ struct page *swapin_readahead(swp_entry_t entry, 
>> >> gfp_t gfp_mask,
>> >>   /* Read a page_cluster sized and aligned cluster around offset. */
>> >>   start_offset = offset & ~mask;
>> >>   end_offset = offset | mask;
>> >> - if (!start_offset)  /* First page is swap header. */
>> >> - start_offset++;
>> >>  
>> >>   blk_start_plug();
>> >>   for (offset = start_offset; offset <= end_offset ; offset++) {
>> >> + swp_entry_t ent = swp_entry(swp_type(entry), offset);
>> >> +
>> >> + if (!swap_entry_check(ent))
>> >> + continue;
>> >>   /* Ok, do the async read-ahead now */
>> >>   page = __read_swap_cache_async(
>> >>   swp_entry(swp_type(entry), offset),
>> >> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> >> index 3074b02eaa09..b04cec29c234 100644
>> >> --- a/mm/swapfile.c
>> >> +++ b/mm/swapfile.c
>> >> @@ -1107,6 +1107,27 @@ static struct swap_info_struct 
>> >> *swap_info_get_cont(swp_entry_t entry,
>> >>   return p;
>> >>  }
>> >>  
>> >> +bool swap_entry_check(swp_entry_t entry)
>> >> +{
>> >> + struct swap_info_struct *p;
>> >> + unsigned long offset, type;
>> >> +
>> >> + type = swp_type(entry);
>> >> + if (type >= nr_swapfiles)
>> >> + goto bad_file;
>> >> + p = swap_info[type];
>> >> + offset = swp_offset(entry);
>> >> + if (unlikely(!offset || offset >= p->max))
>> >> + goto out;
>> >> +
>> >> + return true;
>> >> +
>> >> +bad_file:
>> >> + pr_err("%s: %s%08lx\n", __func__, Bad_file, entry.val);
>> >> +out:
>> >> + return false;
>> >> +}
>> >> +
>> >>  static unsigned char __swap_entry_free(struct swap_info_struct *p,
>> >>  swp_entry_t entry, unsigned char usage)
>> >>  {
>> >> -- 
>> >> 2.14.2
>> >
>> > Although it's better than old, we can make it simple, still.
>> >
>> > diff --git a/include/linux/swapops.h b/include/linux/swapops.h
>> > index 291c4b534658..f50d5a48f03a 100644
>> > --- a/include/linux/swapops.h
>> > +++ b/include/linux/swapops.h
>> > @@ -41,6 +41,13 @@ static inline unsigned swp_type(swp_entry_t entry)
>> >return (entry.val >> SWP_TYPE_SHIFT(entry));
>> >  }
>> >  
>> > +extern struct swap_info_struct *swap_info[];
>> > +
>> > +static inline struct swap_info_struct *swp_si(swp_entry_t entry)
>> > +{
>> > +  return swap_info[swp_type(entry)];
>> > +}
>> > +
>> 
>> Why not just use swp_swap_info()?
>
> 

Re: [PATCH -mm -V2] mm, swap: Fix false error message in __swp_swapcount()

[Minchan Kim]

Hi Huang,

On Tue, Oct 31, 2017 at 01:32:32PM +0800, Huang, Ying wrote:
> Hi, Minchan,
> 
> Minchan Kim  writes:
> 
> > Hi Huang,
> >
> > On Fri, Oct 27, 2017 at 01:53:27PM +0800, Huang, Ying wrote:
> >> From: Huang Ying 
> >> 
> >> When a page fault occurs for a swap entry, the physical swap readahead
> >> (not the VMA base swap readahead) may readahead several swap entries
> >> after the fault swap entry.  The readahead algorithm calculates some
> >> of the swap entries to readahead via increasing the offset of the
> >> fault swap entry without checking whether they are beyond the end of
> >> the swap device and it relys on the __swp_swapcount() and
> >> swapcache_prepare() to check it.  Although __swp_swapcount() checks
> >> for the swap entry passed in, it will complain with the error message
> >> as follow for the expected invalid swap entry.  This may make the end
> >> users confused.
> >> 
> >>   swap_info_get: Bad swap offset entry 0200f8a7
> >> 
> >> To fix the false error message, the swap entry checking is added in
> >> swap readahead to avoid to pass the out-bound swap entries and the
> >> swap entry reserved for the swap header to __swp_swapcount() and
> >> swapcache_prepare().
> >> 
> >> Cc: Tim Chen 
> >> Cc: Minchan Kim 
> >> Cc: Michal Hocko 
> >> Cc:  # 4.11-4.13
> >> Reported-by: Christian Kujau 
> >> Fixes: e8c26ab60598 ("mm/swap: skip readahead for unreferenced swap slots")
> >> Signed-off-by: "Huang, Ying" 
> >> ---
> >>  include/linux/swap.h |  1 +
> >>  mm/swap_state.c  |  6 --
> >>  mm/swapfile.c| 21 +
> >>  3 files changed, 26 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/include/linux/swap.h b/include/linux/swap.h
> >> index 84255b3da7c1..43b4b821c805 100644
> >> --- a/include/linux/swap.h
> >> +++ b/include/linux/swap.h
> >> @@ -476,6 +476,7 @@ extern int page_swapcount(struct page *);
> >>  extern int __swap_count(struct swap_info_struct *si, swp_entry_t entry);
> >>  extern int __swp_swapcount(swp_entry_t entry);
> >>  extern int swp_swapcount(swp_entry_t entry);
> >> +extern bool swap_entry_check(swp_entry_t entry);
> >>  extern struct swap_info_struct *page_swap_info(struct page *);
> >>  extern struct swap_info_struct *swp_swap_info(swp_entry_t entry);
> >>  extern bool reuse_swap_page(struct page *, int *);
> >> diff --git a/mm/swap_state.c b/mm/swap_state.c
> >> index 6c017ced11e6..7dd70e77058d 100644
> >> --- a/mm/swap_state.c
> >> +++ b/mm/swap_state.c
> >> @@ -569,11 +569,13 @@ struct page *swapin_readahead(swp_entry_t entry, 
> >> gfp_t gfp_mask,
> >>/* Read a page_cluster sized and aligned cluster around offset. */
> >>start_offset = offset & ~mask;
> >>end_offset = offset | mask;
> >> -  if (!start_offset)  /* First page is swap header. */
> >> -  start_offset++;
> >>  
> >>blk_start_plug();
> >>for (offset = start_offset; offset <= end_offset ; offset++) {
> >> +  swp_entry_t ent = swp_entry(swp_type(entry), offset);
> >> +
> >> +  if (!swap_entry_check(ent))
> >> +  continue;
> >>/* Ok, do the async read-ahead now */
> >>page = __read_swap_cache_async(
> >>swp_entry(swp_type(entry), offset),
> >> diff --git a/mm/swapfile.c b/mm/swapfile.c
> >> index 3074b02eaa09..b04cec29c234 100644
> >> --- a/mm/swapfile.c
> >> +++ b/mm/swapfile.c
> >> @@ -1107,6 +1107,27 @@ static struct swap_info_struct 
> >> *swap_info_get_cont(swp_entry_t entry,
> >>return p;
> >>  }
> >>  
> >> +bool swap_entry_check(swp_entry_t entry)
> >> +{
> >> +  struct swap_info_struct *p;
> >> +  unsigned long offset, type;
> >> +
> >> +  type = swp_type(entry);
> >> +  if (type >= nr_swapfiles)
> >> +  goto bad_file;
> >> +  p = swap_info[type];
> >> +  offset = swp_offset(entry);
> >> +  if (unlikely(!offset || offset >= p->max))
> >> +  goto out;
> >> +
> >> +  return true;
> >> +
> >> +bad_file:
> >> +  pr_err("%s: %s%08lx\n", __func__, Bad_file, entry.val);
> >> +out:
> >> +  return false;
> >> +}
> >> +
> >>  static unsigned char __swap_entry_free(struct swap_info_struct *p,
> >>   swp_entry_t entry, unsigned char usage)
> >>  {
> >> -- 
> >> 2.14.2
> >
> > Although it's better than old, we can make it simple, still.
> >
> > diff --git a/include/linux/swapops.h b/include/linux/swapops.h
> > index 291c4b534658..f50d5a48f03a 100644
> > --- a/include/linux/swapops.h
> > +++ b/include/linux/swapops.h
> > @@ -41,6 +41,13 @@ static inline unsigned swp_type(swp_entry_t entry)
> > return (entry.val >> SWP_TYPE_SHIFT(entry));
> >  }
> >  
> > +extern struct swap_info_struct *swap_info[];
> > +
> > +static inline struct swap_info_struct *swp_si(swp_entry_t entry)
> > +{
> > +   return swap_info[swp_type(entry)];
> > +}

Re: [PATCH -mm -V2] mm, swap: Fix false error message in __swp_swapcount()

[Minchan Kim]

Hi Huang,

On Tue, Oct 31, 2017 at 01:32:32PM +0800, Huang, Ying wrote:
> Hi, Minchan,
> 
> Minchan Kim  writes:
> 
> > Hi Huang,
> >
> > On Fri, Oct 27, 2017 at 01:53:27PM +0800, Huang, Ying wrote:
> >> From: Huang Ying 
> >> 
> >> When a page fault occurs for a swap entry, the physical swap readahead
> >> (not the VMA base swap readahead) may readahead several swap entries
> >> after the fault swap entry.  The readahead algorithm calculates some
> >> of the swap entries to readahead via increasing the offset of the
> >> fault swap entry without checking whether they are beyond the end of
> >> the swap device and it relys on the __swp_swapcount() and
> >> swapcache_prepare() to check it.  Although __swp_swapcount() checks
> >> for the swap entry passed in, it will complain with the error message
> >> as follow for the expected invalid swap entry.  This may make the end
> >> users confused.
> >> 
> >>   swap_info_get: Bad swap offset entry 0200f8a7
> >> 
> >> To fix the false error message, the swap entry checking is added in
> >> swap readahead to avoid to pass the out-bound swap entries and the
> >> swap entry reserved for the swap header to __swp_swapcount() and
> >> swapcache_prepare().
> >> 
> >> Cc: Tim Chen 
> >> Cc: Minchan Kim 
> >> Cc: Michal Hocko 
> >> Cc:  # 4.11-4.13
> >> Reported-by: Christian Kujau 
> >> Fixes: e8c26ab60598 ("mm/swap: skip readahead for unreferenced swap slots")
> >> Signed-off-by: "Huang, Ying" 
> >> ---
> >>  include/linux/swap.h |  1 +
> >>  mm/swap_state.c  |  6 --
> >>  mm/swapfile.c| 21 +
> >>  3 files changed, 26 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/include/linux/swap.h b/include/linux/swap.h
> >> index 84255b3da7c1..43b4b821c805 100644
> >> --- a/include/linux/swap.h
> >> +++ b/include/linux/swap.h
> >> @@ -476,6 +476,7 @@ extern int page_swapcount(struct page *);
> >>  extern int __swap_count(struct swap_info_struct *si, swp_entry_t entry);
> >>  extern int __swp_swapcount(swp_entry_t entry);
> >>  extern int swp_swapcount(swp_entry_t entry);
> >> +extern bool swap_entry_check(swp_entry_t entry);
> >>  extern struct swap_info_struct *page_swap_info(struct page *);
> >>  extern struct swap_info_struct *swp_swap_info(swp_entry_t entry);
> >>  extern bool reuse_swap_page(struct page *, int *);
> >> diff --git a/mm/swap_state.c b/mm/swap_state.c
> >> index 6c017ced11e6..7dd70e77058d 100644
> >> --- a/mm/swap_state.c
> >> +++ b/mm/swap_state.c
> >> @@ -569,11 +569,13 @@ struct page *swapin_readahead(swp_entry_t entry, 
> >> gfp_t gfp_mask,
> >>/* Read a page_cluster sized and aligned cluster around offset. */
> >>start_offset = offset & ~mask;
> >>end_offset = offset | mask;
> >> -  if (!start_offset)  /* First page is swap header. */
> >> -  start_offset++;
> >>  
> >>blk_start_plug();
> >>for (offset = start_offset; offset <= end_offset ; offset++) {
> >> +  swp_entry_t ent = swp_entry(swp_type(entry), offset);
> >> +
> >> +  if (!swap_entry_check(ent))
> >> +  continue;
> >>/* Ok, do the async read-ahead now */
> >>page = __read_swap_cache_async(
> >>swp_entry(swp_type(entry), offset),
> >> diff --git a/mm/swapfile.c b/mm/swapfile.c
> >> index 3074b02eaa09..b04cec29c234 100644
> >> --- a/mm/swapfile.c
> >> +++ b/mm/swapfile.c
> >> @@ -1107,6 +1107,27 @@ static struct swap_info_struct 
> >> *swap_info_get_cont(swp_entry_t entry,
> >>return p;
> >>  }
> >>  
> >> +bool swap_entry_check(swp_entry_t entry)
> >> +{
> >> +  struct swap_info_struct *p;
> >> +  unsigned long offset, type;
> >> +
> >> +  type = swp_type(entry);
> >> +  if (type >= nr_swapfiles)
> >> +  goto bad_file;
> >> +  p = swap_info[type];
> >> +  offset = swp_offset(entry);
> >> +  if (unlikely(!offset || offset >= p->max))
> >> +  goto out;
> >> +
> >> +  return true;
> >> +
> >> +bad_file:
> >> +  pr_err("%s: %s%08lx\n", __func__, Bad_file, entry.val);
> >> +out:
> >> +  return false;
> >> +}
> >> +
> >>  static unsigned char __swap_entry_free(struct swap_info_struct *p,
> >>   swp_entry_t entry, unsigned char usage)
> >>  {
> >> -- 
> >> 2.14.2
> >
> > Although it's better than old, we can make it simple, still.
> >
> > diff --git a/include/linux/swapops.h b/include/linux/swapops.h
> > index 291c4b534658..f50d5a48f03a 100644
> > --- a/include/linux/swapops.h
> > +++ b/include/linux/swapops.h
> > @@ -41,6 +41,13 @@ static inline unsigned swp_type(swp_entry_t entry)
> > return (entry.val >> SWP_TYPE_SHIFT(entry));
> >  }
> >  
> > +extern struct swap_info_struct *swap_info[];
> > +
> > +static inline struct swap_info_struct *swp_si(swp_entry_t entry)
> > +{
> > +   return swap_info[swp_type(entry)];
> > +}
> > +
> 
> Why not just use swp_swap_info()?

Yeap, I forgot the one that even I added the function. :(

> 
> >  /*
> >   * Extract the `offset' field from a swp_entry_t.  The 

Re: [PATCH -mm -V2] mm, swap: Fix false error message in __swp_swapcount()

[Huang, Ying]

Hi, Minchan,

Minchan Kim  writes:

> Hi Huang,
>
> On Fri, Oct 27, 2017 at 01:53:27PM +0800, Huang, Ying wrote:
>> From: Huang Ying 
>> 
>> When a page fault occurs for a swap entry, the physical swap readahead
>> (not the VMA base swap readahead) may readahead several swap entries
>> after the fault swap entry.  The readahead algorithm calculates some
>> of the swap entries to readahead via increasing the offset of the
>> fault swap entry without checking whether they are beyond the end of
>> the swap device and it relys on the __swp_swapcount() and
>> swapcache_prepare() to check it.  Although __swp_swapcount() checks
>> for the swap entry passed in, it will complain with the error message
>> as follow for the expected invalid swap entry.  This may make the end
>> users confused.
>> 
>>   swap_info_get: Bad swap offset entry 0200f8a7
>> 
>> To fix the false error message, the swap entry checking is added in
>> swap readahead to avoid to pass the out-bound swap entries and the
>> swap entry reserved for the swap header to __swp_swapcount() and
>> swapcache_prepare().
>> 
>> Cc: Tim Chen 
>> Cc: Minchan Kim 
>> Cc: Michal Hocko 
>> Cc:  # 4.11-4.13
>> Reported-by: Christian Kujau 
>> Fixes: e8c26ab60598 ("mm/swap: skip readahead for unreferenced swap slots")
>> Signed-off-by: "Huang, Ying" 
>> ---
>>  include/linux/swap.h |  1 +
>>  mm/swap_state.c  |  6 --
>>  mm/swapfile.c| 21 +
>>  3 files changed, 26 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/linux/swap.h b/include/linux/swap.h
>> index 84255b3da7c1..43b4b821c805 100644
>> --- a/include/linux/swap.h
>> +++ b/include/linux/swap.h
>> @@ -476,6 +476,7 @@ extern int page_swapcount(struct page *);
>>  extern int __swap_count(struct swap_info_struct *si, swp_entry_t entry);
>>  extern int __swp_swapcount(swp_entry_t entry);
>>  extern int swp_swapcount(swp_entry_t entry);
>> +extern bool swap_entry_check(swp_entry_t entry);
>>  extern struct swap_info_struct *page_swap_info(struct page *);
>>  extern struct swap_info_struct *swp_swap_info(swp_entry_t entry);
>>  extern bool reuse_swap_page(struct page *, int *);
>> diff --git a/mm/swap_state.c b/mm/swap_state.c
>> index 6c017ced11e6..7dd70e77058d 100644
>> --- a/mm/swap_state.c
>> +++ b/mm/swap_state.c
>> @@ -569,11 +569,13 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t 
>> gfp_mask,
>>  /* Read a page_cluster sized and aligned cluster around offset. */
>>  start_offset = offset & ~mask;
>>  end_offset = offset | mask;
>> -if (!start_offset)  /* First page is swap header. */
>> -start_offset++;
>>  
>>  blk_start_plug();
>>  for (offset = start_offset; offset <= end_offset ; offset++) {
>> +swp_entry_t ent = swp_entry(swp_type(entry), offset);
>> +
>> +if (!swap_entry_check(ent))
>> +continue;
>>  /* Ok, do the async read-ahead now */
>>  page = __read_swap_cache_async(
>>  swp_entry(swp_type(entry), offset),
>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> index 3074b02eaa09..b04cec29c234 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -1107,6 +1107,27 @@ static struct swap_info_struct 
>> *swap_info_get_cont(swp_entry_t entry,
>>  return p;
>>  }
>>  
>> +bool swap_entry_check(swp_entry_t entry)
>> +{
>> +struct swap_info_struct *p;
>> +unsigned long offset, type;
>> +
>> +type = swp_type(entry);
>> +if (type >= nr_swapfiles)
>> +goto bad_file;
>> +p = swap_info[type];
>> +offset = swp_offset(entry);
>> +if (unlikely(!offset || offset >= p->max))
>> +goto out;
>> +
>> +return true;
>> +
>> +bad_file:
>> +pr_err("%s: %s%08lx\n", __func__, Bad_file, entry.val);
>> +out:
>> +return false;
>> +}
>> +
>>  static unsigned char __swap_entry_free(struct swap_info_struct *p,
>> swp_entry_t entry, unsigned char usage)
>>  {
>> -- 
>> 2.14.2
>
> Although it's better than old, we can make it simple, still.
>
> diff --git a/include/linux/swapops.h b/include/linux/swapops.h
> index 291c4b534658..f50d5a48f03a 100644
> --- a/include/linux/swapops.h
> +++ b/include/linux/swapops.h
> @@ -41,6 +41,13 @@ static inline unsigned swp_type(swp_entry_t entry)
>   return (entry.val >> SWP_TYPE_SHIFT(entry));
>  }
>  
> +extern struct swap_info_struct *swap_info[];
> +
> +static inline struct swap_info_struct *swp_si(swp_entry_t entry)
> +{
> + return swap_info[swp_type(entry)];
> +}
> +

Why not just use swp_swap_info()?

>  /*
>   * Extract the `offset' field from a swp_entry_t.  The swp_entry_t is in
>   * arch-independent format
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 378262d3a197..a0fe2d54ad09 100644
> --- 

Re: [PATCH -mm -V2] mm, swap: Fix false error message in __swp_swapcount()

[Huang, Ying]

Hi, Minchan,

Minchan Kim  writes:

> Hi Huang,
>
> On Fri, Oct 27, 2017 at 01:53:27PM +0800, Huang, Ying wrote:
>> From: Huang Ying 
>> 
>> When a page fault occurs for a swap entry, the physical swap readahead
>> (not the VMA base swap readahead) may readahead several swap entries
>> after the fault swap entry.  The readahead algorithm calculates some
>> of the swap entries to readahead via increasing the offset of the
>> fault swap entry without checking whether they are beyond the end of
>> the swap device and it relys on the __swp_swapcount() and
>> swapcache_prepare() to check it.  Although __swp_swapcount() checks
>> for the swap entry passed in, it will complain with the error message
>> as follow for the expected invalid swap entry.  This may make the end
>> users confused.
>> 
>>   swap_info_get: Bad swap offset entry 0200f8a7
>> 
>> To fix the false error message, the swap entry checking is added in
>> swap readahead to avoid to pass the out-bound swap entries and the
>> swap entry reserved for the swap header to __swp_swapcount() and
>> swapcache_prepare().
>> 
>> Cc: Tim Chen 
>> Cc: Minchan Kim 
>> Cc: Michal Hocko 
>> Cc:  # 4.11-4.13
>> Reported-by: Christian Kujau 
>> Fixes: e8c26ab60598 ("mm/swap: skip readahead for unreferenced swap slots")
>> Signed-off-by: "Huang, Ying" 
>> ---
>>  include/linux/swap.h |  1 +
>>  mm/swap_state.c  |  6 --
>>  mm/swapfile.c| 21 +
>>  3 files changed, 26 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/linux/swap.h b/include/linux/swap.h
>> index 84255b3da7c1..43b4b821c805 100644
>> --- a/include/linux/swap.h
>> +++ b/include/linux/swap.h
>> @@ -476,6 +476,7 @@ extern int page_swapcount(struct page *);
>>  extern int __swap_count(struct swap_info_struct *si, swp_entry_t entry);
>>  extern int __swp_swapcount(swp_entry_t entry);
>>  extern int swp_swapcount(swp_entry_t entry);
>> +extern bool swap_entry_check(swp_entry_t entry);
>>  extern struct swap_info_struct *page_swap_info(struct page *);
>>  extern struct swap_info_struct *swp_swap_info(swp_entry_t entry);
>>  extern bool reuse_swap_page(struct page *, int *);
>> diff --git a/mm/swap_state.c b/mm/swap_state.c
>> index 6c017ced11e6..7dd70e77058d 100644
>> --- a/mm/swap_state.c
>> +++ b/mm/swap_state.c
>> @@ -569,11 +569,13 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t 
>> gfp_mask,
>>  /* Read a page_cluster sized and aligned cluster around offset. */
>>  start_offset = offset & ~mask;
>>  end_offset = offset | mask;
>> -if (!start_offset)  /* First page is swap header. */
>> -start_offset++;
>>  
>>  blk_start_plug();
>>  for (offset = start_offset; offset <= end_offset ; offset++) {
>> +swp_entry_t ent = swp_entry(swp_type(entry), offset);
>> +
>> +if (!swap_entry_check(ent))
>> +continue;
>>  /* Ok, do the async read-ahead now */
>>  page = __read_swap_cache_async(
>>  swp_entry(swp_type(entry), offset),
>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> index 3074b02eaa09..b04cec29c234 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -1107,6 +1107,27 @@ static struct swap_info_struct 
>> *swap_info_get_cont(swp_entry_t entry,
>>  return p;
>>  }
>>  
>> +bool swap_entry_check(swp_entry_t entry)
>> +{
>> +struct swap_info_struct *p;
>> +unsigned long offset, type;
>> +
>> +type = swp_type(entry);
>> +if (type >= nr_swapfiles)
>> +goto bad_file;
>> +p = swap_info[type];
>> +offset = swp_offset(entry);
>> +if (unlikely(!offset || offset >= p->max))
>> +goto out;
>> +
>> +return true;
>> +
>> +bad_file:
>> +pr_err("%s: %s%08lx\n", __func__, Bad_file, entry.val);
>> +out:
>> +return false;
>> +}
>> +
>>  static unsigned char __swap_entry_free(struct swap_info_struct *p,
>> swp_entry_t entry, unsigned char usage)
>>  {
>> -- 
>> 2.14.2
>
> Although it's better than old, we can make it simple, still.
>
> diff --git a/include/linux/swapops.h b/include/linux/swapops.h
> index 291c4b534658..f50d5a48f03a 100644
> --- a/include/linux/swapops.h
> +++ b/include/linux/swapops.h
> @@ -41,6 +41,13 @@ static inline unsigned swp_type(swp_entry_t entry)
>   return (entry.val >> SWP_TYPE_SHIFT(entry));
>  }
>  
> +extern struct swap_info_struct *swap_info[];
> +
> +static inline struct swap_info_struct *swp_si(swp_entry_t entry)
> +{
> + return swap_info[swp_type(entry)];
> +}
> +

Why not just use swp_swap_info()?

>  /*
>   * Extract the `offset' field from a swp_entry_t.  The swp_entry_t is in
>   * arch-independent format
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 378262d3a197..a0fe2d54ad09 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -554,6 +554,7 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t 
> gfp_mask,
>   struct vm_area_struct *vma, unsigned long 

Re: [PATCH -mm -V2] mm, swap: Fix false error message in __swp_swapcount()

[Minchan Kim]

On Mon, Oct 30, 2017 at 09:02:30AM +0100, Michal Hocko wrote:
> On Mon 30-10-17 08:57:13, Minchan Kim wrote:
> [...]
> > Although it's better than old, we can make it simple, still.
> > 
> > diff --git a/include/linux/swapops.h b/include/linux/swapops.h
> > index 291c4b534658..f50d5a48f03a 100644
> > --- a/include/linux/swapops.h
> > +++ b/include/linux/swapops.h
> > @@ -41,6 +41,13 @@ static inline unsigned swp_type(swp_entry_t entry)
> > return (entry.val >> SWP_TYPE_SHIFT(entry));
> >  }
> >  
> > +extern struct swap_info_struct *swap_info[];
> > +
> > +static inline struct swap_info_struct *swp_si(swp_entry_t entry)
> > +{
> > +   return swap_info[swp_type(entry)];
> > +}
> > +
> >  /*
> >   * Extract the `offset' field from a swp_entry_t.  The swp_entry_t is in
> >   * arch-independent format
> > diff --git a/mm/swap_state.c b/mm/swap_state.c
> > index 378262d3a197..a0fe2d54ad09 100644
> > --- a/mm/swap_state.c
> > +++ b/mm/swap_state.c
> > @@ -554,6 +554,7 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t 
> > gfp_mask,
> > struct vm_area_struct *vma, unsigned long addr)
> >  {
> > struct page *page;
> > +   struct swap_info_struct *si = swp_si(entry);
> 
> Aren't you accessing beyond the array here?

I couldn't understand what you intend. Could you explain what case does it 
accesses
beyond the arrary?

Thanks.

> 
> > unsigned long entry_offset = swp_offset(entry);
> > unsigned long offset = entry_offset;
> > unsigned long start_offset, end_offset;
> > @@ -572,6 +573,9 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t 
> > gfp_mask,
> > if (!start_offset)  /* First page is swap header. */
> > start_offset++;
> >  
> > +   if (end_offset >= si->max)
> > +   end_offset = si->max - 1;
> > +
> > blk_start_plug();
> > for (offset = start_offset; offset <= end_offset ; offset++) {
> > /* Ok, do the async read-ahead now */
> 
> -- 
> Michal Hocko
> SUSE Labs
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majord...@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: mailto:"d...@kvack.org;> em...@kvack.org 

Re: [PATCH -mm -V2] mm, swap: Fix false error message in __swp_swapcount()

[Minchan Kim]

On Mon, Oct 30, 2017 at 09:02:30AM +0100, Michal Hocko wrote:
> On Mon 30-10-17 08:57:13, Minchan Kim wrote:
> [...]
> > Although it's better than old, we can make it simple, still.
> > 
> > diff --git a/include/linux/swapops.h b/include/linux/swapops.h
> > index 291c4b534658..f50d5a48f03a 100644
> > --- a/include/linux/swapops.h
> > +++ b/include/linux/swapops.h
> > @@ -41,6 +41,13 @@ static inline unsigned swp_type(swp_entry_t entry)
> > return (entry.val >> SWP_TYPE_SHIFT(entry));
> >  }
> >  
> > +extern struct swap_info_struct *swap_info[];
> > +
> > +static inline struct swap_info_struct *swp_si(swp_entry_t entry)
> > +{
> > +   return swap_info[swp_type(entry)];
> > +}
> > +
> >  /*
> >   * Extract the `offset' field from a swp_entry_t.  The swp_entry_t is in
> >   * arch-independent format
> > diff --git a/mm/swap_state.c b/mm/swap_state.c
> > index 378262d3a197..a0fe2d54ad09 100644
> > --- a/mm/swap_state.c
> > +++ b/mm/swap_state.c
> > @@ -554,6 +554,7 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t 
> > gfp_mask,
> > struct vm_area_struct *vma, unsigned long addr)
> >  {
> > struct page *page;
> > +   struct swap_info_struct *si = swp_si(entry);
> 
> Aren't you accessing beyond the array here?

I couldn't understand what you intend. Could you explain what case does it 
accesses
beyond the arrary?

Thanks.

> 
> > unsigned long entry_offset = swp_offset(entry);
> > unsigned long offset = entry_offset;
> > unsigned long start_offset, end_offset;
> > @@ -572,6 +573,9 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t 
> > gfp_mask,
> > if (!start_offset)  /* First page is swap header. */
> > start_offset++;
> >  
> > +   if (end_offset >= si->max)
> > +   end_offset = si->max - 1;
> > +
> > blk_start_plug();
> > for (offset = start_offset; offset <= end_offset ; offset++) {
> > /* Ok, do the async read-ahead now */
> 
> -- 
> Michal Hocko
> SUSE Labs
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majord...@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: mailto:"d...@kvack.org;> em...@kvack.org 

Re: [PATCH -mm -V2] mm, swap: Fix false error message in __swp_swapcount()

[Michal Hocko]

On Mon 30-10-17 08:57:13, Minchan Kim wrote:
[...]
> Although it's better than old, we can make it simple, still.
> 
> diff --git a/include/linux/swapops.h b/include/linux/swapops.h
> index 291c4b534658..f50d5a48f03a 100644
> --- a/include/linux/swapops.h
> +++ b/include/linux/swapops.h
> @@ -41,6 +41,13 @@ static inline unsigned swp_type(swp_entry_t entry)
>   return (entry.val >> SWP_TYPE_SHIFT(entry));
>  }
>  
> +extern struct swap_info_struct *swap_info[];
> +
> +static inline struct swap_info_struct *swp_si(swp_entry_t entry)
> +{
> + return swap_info[swp_type(entry)];
> +}
> +
>  /*
>   * Extract the `offset' field from a swp_entry_t.  The swp_entry_t is in
>   * arch-independent format
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 378262d3a197..a0fe2d54ad09 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -554,6 +554,7 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t 
> gfp_mask,
>   struct vm_area_struct *vma, unsigned long addr)
>  {
>   struct page *page;
> + struct swap_info_struct *si = swp_si(entry);

Aren't you accessing beyond the array here?

>   unsigned long entry_offset = swp_offset(entry);
>   unsigned long offset = entry_offset;
>   unsigned long start_offset, end_offset;
> @@ -572,6 +573,9 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t 
> gfp_mask,
>   if (!start_offset)  /* First page is swap header. */
>   start_offset++;
>  
> + if (end_offset >= si->max)
> + end_offset = si->max - 1;
> +
>   blk_start_plug();
>   for (offset = start_offset; offset <= end_offset ; offset++) {
>   /* Ok, do the async read-ahead now */

-- 
Michal Hocko
SUSE Labs

Re: [PATCH -mm -V2] mm, swap: Fix false error message in __swp_swapcount()

[Michal Hocko]

On Mon 30-10-17 08:57:13, Minchan Kim wrote:
[...]
> Although it's better than old, we can make it simple, still.
> 
> diff --git a/include/linux/swapops.h b/include/linux/swapops.h
> index 291c4b534658..f50d5a48f03a 100644
> --- a/include/linux/swapops.h
> +++ b/include/linux/swapops.h
> @@ -41,6 +41,13 @@ static inline unsigned swp_type(swp_entry_t entry)
>   return (entry.val >> SWP_TYPE_SHIFT(entry));
>  }
>  
> +extern struct swap_info_struct *swap_info[];
> +
> +static inline struct swap_info_struct *swp_si(swp_entry_t entry)
> +{
> + return swap_info[swp_type(entry)];
> +}
> +
>  /*
>   * Extract the `offset' field from a swp_entry_t.  The swp_entry_t is in
>   * arch-independent format
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 378262d3a197..a0fe2d54ad09 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -554,6 +554,7 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t 
> gfp_mask,
>   struct vm_area_struct *vma, unsigned long addr)
>  {
>   struct page *page;
> + struct swap_info_struct *si = swp_si(entry);

Aren't you accessing beyond the array here?

>   unsigned long entry_offset = swp_offset(entry);
>   unsigned long offset = entry_offset;
>   unsigned long start_offset, end_offset;
> @@ -572,6 +573,9 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t 
> gfp_mask,
>   if (!start_offset)  /* First page is swap header. */
>   start_offset++;
>  
> + if (end_offset >= si->max)
> + end_offset = si->max - 1;
> +
>   blk_start_plug();
>   for (offset = start_offset; offset <= end_offset ; offset++) {
>   /* Ok, do the async read-ahead now */

-- 
Michal Hocko
SUSE Labs

Re: [PATCH -mm -V2] mm, swap: Fix false error message in __swp_swapcount()

[Minchan Kim]

Hi Huang,

On Fri, Oct 27, 2017 at 01:53:27PM +0800, Huang, Ying wrote:
> From: Huang Ying 
> 
> When a page fault occurs for a swap entry, the physical swap readahead
> (not the VMA base swap readahead) may readahead several swap entries
> after the fault swap entry.  The readahead algorithm calculates some
> of the swap entries to readahead via increasing the offset of the
> fault swap entry without checking whether they are beyond the end of
> the swap device and it relys on the __swp_swapcount() and
> swapcache_prepare() to check it.  Although __swp_swapcount() checks
> for the swap entry passed in, it will complain with the error message
> as follow for the expected invalid swap entry.  This may make the end
> users confused.
> 
>   swap_info_get: Bad swap offset entry 0200f8a7
> 
> To fix the false error message, the swap entry checking is added in
> swap readahead to avoid to pass the out-bound swap entries and the
> swap entry reserved for the swap header to __swp_swapcount() and
> swapcache_prepare().
> 
> Cc: Tim Chen 
> Cc: Minchan Kim 
> Cc: Michal Hocko 
> Cc:  # 4.11-4.13
> Reported-by: Christian Kujau 
> Fixes: e8c26ab60598 ("mm/swap: skip readahead for unreferenced swap slots")
> Signed-off-by: "Huang, Ying" 
> ---
>  include/linux/swap.h |  1 +
>  mm/swap_state.c  |  6 --
>  mm/swapfile.c| 21 +
>  3 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index 84255b3da7c1..43b4b821c805 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -476,6 +476,7 @@ extern int page_swapcount(struct page *);
>  extern int __swap_count(struct swap_info_struct *si, swp_entry_t entry);
>  extern int __swp_swapcount(swp_entry_t entry);
>  extern int swp_swapcount(swp_entry_t entry);
> +extern bool swap_entry_check(swp_entry_t entry);
>  extern struct swap_info_struct *page_swap_info(struct page *);
>  extern struct swap_info_struct *swp_swap_info(swp_entry_t entry);
>  extern bool reuse_swap_page(struct page *, int *);
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 6c017ced11e6..7dd70e77058d 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -569,11 +569,13 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t 
> gfp_mask,
>   /* Read a page_cluster sized and aligned cluster around offset. */
>   start_offset = offset & ~mask;
>   end_offset = offset | mask;
> - if (!start_offset)  /* First page is swap header. */
> - start_offset++;
>  
>   blk_start_plug();
>   for (offset = start_offset; offset <= end_offset ; offset++) {
> + swp_entry_t ent = swp_entry(swp_type(entry), offset);
> +
> + if (!swap_entry_check(ent))
> + continue;
>   /* Ok, do the async read-ahead now */
>   page = __read_swap_cache_async(
>   swp_entry(swp_type(entry), offset),
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 3074b02eaa09..b04cec29c234 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1107,6 +1107,27 @@ static struct swap_info_struct 
> *swap_info_get_cont(swp_entry_t entry,
>   return p;
>  }
>  
> +bool swap_entry_check(swp_entry_t entry)
> +{
> + struct swap_info_struct *p;
> + unsigned long offset, type;
> +
> + type = swp_type(entry);
> + if (type >= nr_swapfiles)
> + goto bad_file;
> + p = swap_info[type];
> + offset = swp_offset(entry);
> + if (unlikely(!offset || offset >= p->max))
> + goto out;
> +
> + return true;
> +
> +bad_file:
> + pr_err("%s: %s%08lx\n", __func__, Bad_file, entry.val);
> +out:
> + return false;
> +}
> +
>  static unsigned char __swap_entry_free(struct swap_info_struct *p,
>  swp_entry_t entry, unsigned char usage)
>  {
> -- 
> 2.14.2

Although it's better than old, we can make it simple, still.

diff --git a/include/linux/swapops.h b/include/linux/swapops.h
index 291c4b534658..f50d5a48f03a 100644
--- a/include/linux/swapops.h
+++ b/include/linux/swapops.h
@@ -41,6 +41,13 @@ static inline unsigned swp_type(swp_entry_t entry)
return (entry.val >> SWP_TYPE_SHIFT(entry));
 }
 
+extern struct swap_info_struct *swap_info[];
+
+static inline struct swap_info_struct *swp_si(swp_entry_t entry)
+{
+   return swap_info[swp_type(entry)];
+}
+
 /*
  * Extract the `offset' field from a swp_entry_t.  The swp_entry_t is in
  * arch-independent format
diff --git a/mm/swap_state.c b/mm/swap_state.c
index 378262d3a197..a0fe2d54ad09 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -554,6 +554,7 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t 
gfp_mask,
struct vm_area_struct *vma, unsigned long addr)
 {
struct page *page;

Re: [PATCH -mm -V2] mm, swap: Fix false error message in __swp_swapcount()

[Minchan Kim]

Hi Huang,

On Fri, Oct 27, 2017 at 01:53:27PM +0800, Huang, Ying wrote:
> From: Huang Ying 
> 
> When a page fault occurs for a swap entry, the physical swap readahead
> (not the VMA base swap readahead) may readahead several swap entries
> after the fault swap entry.  The readahead algorithm calculates some
> of the swap entries to readahead via increasing the offset of the
> fault swap entry without checking whether they are beyond the end of
> the swap device and it relys on the __swp_swapcount() and
> swapcache_prepare() to check it.  Although __swp_swapcount() checks
> for the swap entry passed in, it will complain with the error message
> as follow for the expected invalid swap entry.  This may make the end
> users confused.
> 
>   swap_info_get: Bad swap offset entry 0200f8a7
> 
> To fix the false error message, the swap entry checking is added in
> swap readahead to avoid to pass the out-bound swap entries and the
> swap entry reserved for the swap header to __swp_swapcount() and
> swapcache_prepare().
> 
> Cc: Tim Chen 
> Cc: Minchan Kim 
> Cc: Michal Hocko 
> Cc:  # 4.11-4.13
> Reported-by: Christian Kujau 
> Fixes: e8c26ab60598 ("mm/swap: skip readahead for unreferenced swap slots")
> Signed-off-by: "Huang, Ying" 
> ---
>  include/linux/swap.h |  1 +
>  mm/swap_state.c  |  6 --
>  mm/swapfile.c| 21 +
>  3 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index 84255b3da7c1..43b4b821c805 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -476,6 +476,7 @@ extern int page_swapcount(struct page *);
>  extern int __swap_count(struct swap_info_struct *si, swp_entry_t entry);
>  extern int __swp_swapcount(swp_entry_t entry);
>  extern int swp_swapcount(swp_entry_t entry);
> +extern bool swap_entry_check(swp_entry_t entry);
>  extern struct swap_info_struct *page_swap_info(struct page *);
>  extern struct swap_info_struct *swp_swap_info(swp_entry_t entry);
>  extern bool reuse_swap_page(struct page *, int *);
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 6c017ced11e6..7dd70e77058d 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -569,11 +569,13 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t 
> gfp_mask,
>   /* Read a page_cluster sized and aligned cluster around offset. */
>   start_offset = offset & ~mask;
>   end_offset = offset | mask;
> - if (!start_offset)  /* First page is swap header. */
> - start_offset++;
>  
>   blk_start_plug();
>   for (offset = start_offset; offset <= end_offset ; offset++) {
> + swp_entry_t ent = swp_entry(swp_type(entry), offset);
> +
> + if (!swap_entry_check(ent))
> + continue;
>   /* Ok, do the async read-ahead now */
>   page = __read_swap_cache_async(
>   swp_entry(swp_type(entry), offset),
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 3074b02eaa09..b04cec29c234 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1107,6 +1107,27 @@ static struct swap_info_struct 
> *swap_info_get_cont(swp_entry_t entry,
>   return p;
>  }
>  
> +bool swap_entry_check(swp_entry_t entry)
> +{
> + struct swap_info_struct *p;
> + unsigned long offset, type;
> +
> + type = swp_type(entry);
> + if (type >= nr_swapfiles)
> + goto bad_file;
> + p = swap_info[type];
> + offset = swp_offset(entry);
> + if (unlikely(!offset || offset >= p->max))
> + goto out;
> +
> + return true;
> +
> +bad_file:
> + pr_err("%s: %s%08lx\n", __func__, Bad_file, entry.val);
> +out:
> + return false;
> +}
> +
>  static unsigned char __swap_entry_free(struct swap_info_struct *p,
>  swp_entry_t entry, unsigned char usage)
>  {
> -- 
> 2.14.2

Although it's better than old, we can make it simple, still.

diff --git a/include/linux/swapops.h b/include/linux/swapops.h
index 291c4b534658..f50d5a48f03a 100644
--- a/include/linux/swapops.h
+++ b/include/linux/swapops.h
@@ -41,6 +41,13 @@ static inline unsigned swp_type(swp_entry_t entry)
return (entry.val >> SWP_TYPE_SHIFT(entry));
 }
 
+extern struct swap_info_struct *swap_info[];
+
+static inline struct swap_info_struct *swp_si(swp_entry_t entry)
+{
+   return swap_info[swp_type(entry)];
+}
+
 /*
  * Extract the `offset' field from a swp_entry_t.  The swp_entry_t is in
  * arch-independent format
diff --git a/mm/swap_state.c b/mm/swap_state.c
index 378262d3a197..a0fe2d54ad09 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -554,6 +554,7 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t 
gfp_mask,
struct vm_area_struct *vma, unsigned long addr)
 {
struct page *page;
+   struct swap_info_struct *si = swp_si(entry);
unsigned long entry_offset = swp_offset(entry);
unsigned long offset = entry_offset;

Re: [PATCH -mm -V2] mm, swap: Fix false error message in __swp_swapcount()

[Michal Hocko]

On Fri 27-10-17 13:53:27, Huang, Ying wrote:
> From: Huang Ying 
> 
> When a page fault occurs for a swap entry, the physical swap readahead
> (not the VMA base swap readahead) may readahead several swap entries
> after the fault swap entry.  The readahead algorithm calculates some
> of the swap entries to readahead via increasing the offset of the
> fault swap entry without checking whether they are beyond the end of
> the swap device and it relys on the __swp_swapcount() and
> swapcache_prepare() to check it.  Although __swp_swapcount() checks
> for the swap entry passed in, it will complain with the error message
> as follow for the expected invalid swap entry.  This may make the end
> users confused.
> 
>   swap_info_get: Bad swap offset entry 0200f8a7
> 
> To fix the false error message, the swap entry checking is added in
> swap readahead to avoid to pass the out-bound swap entries and the
> swap entry reserved for the swap header to __swp_swapcount() and
> swapcache_prepare().

I have lost an overview in the swap code after recent changes so I
cannot really give you a responsible ack but this looks much better
than the previous attempt. So it looks good at first sight.

> Cc: Tim Chen 
> Cc: Minchan Kim 
> Cc: Michal Hocko 
> Cc:  # 4.11-4.13
> Reported-by: Christian Kujau 
> Fixes: e8c26ab60598 ("mm/swap: skip readahead for unreferenced swap slots")
> Signed-off-by: "Huang, Ying" 
> ---
>  include/linux/swap.h |  1 +
>  mm/swap_state.c  |  6 --
>  mm/swapfile.c| 21 +
>  3 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index 84255b3da7c1..43b4b821c805 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -476,6 +476,7 @@ extern int page_swapcount(struct page *);
>  extern int __swap_count(struct swap_info_struct *si, swp_entry_t entry);
>  extern int __swp_swapcount(swp_entry_t entry);
>  extern int swp_swapcount(swp_entry_t entry);
> +extern bool swap_entry_check(swp_entry_t entry);
>  extern struct swap_info_struct *page_swap_info(struct page *);
>  extern struct swap_info_struct *swp_swap_info(swp_entry_t entry);
>  extern bool reuse_swap_page(struct page *, int *);
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 6c017ced11e6..7dd70e77058d 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -569,11 +569,13 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t 
> gfp_mask,
>   /* Read a page_cluster sized and aligned cluster around offset. */
>   start_offset = offset & ~mask;
>   end_offset = offset | mask;
> - if (!start_offset)  /* First page is swap header. */
> - start_offset++;
>  
>   blk_start_plug();
>   for (offset = start_offset; offset <= end_offset ; offset++) {
> + swp_entry_t ent = swp_entry(swp_type(entry), offset);
> +
> + if (!swap_entry_check(ent))
> + continue;
>   /* Ok, do the async read-ahead now */
>   page = __read_swap_cache_async(
>   swp_entry(swp_type(entry), offset),
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 3074b02eaa09..b04cec29c234 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1107,6 +1107,27 @@ static struct swap_info_struct 
> *swap_info_get_cont(swp_entry_t entry,
>   return p;
>  }
>  
> +bool swap_entry_check(swp_entry_t entry)
> +{
> + struct swap_info_struct *p;
> + unsigned long offset, type;
> +
> + type = swp_type(entry);
> + if (type >= nr_swapfiles)
> + goto bad_file;
> + p = swap_info[type];
> + offset = swp_offset(entry);
> + if (unlikely(!offset || offset >= p->max))
> + goto out;
> +
> + return true;
> +
> +bad_file:
> + pr_err("%s: %s%08lx\n", __func__, Bad_file, entry.val);
> +out:
> + return false;
> +}
> +
>  static unsigned char __swap_entry_free(struct swap_info_struct *p,
>  swp_entry_t entry, unsigned char usage)
>  {
> -- 
> 2.14.2
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH -mm -V2] mm, swap: Fix false error message in __swp_swapcount()

[Michal Hocko]

On Fri 27-10-17 13:53:27, Huang, Ying wrote:
> From: Huang Ying 
> 
> When a page fault occurs for a swap entry, the physical swap readahead
> (not the VMA base swap readahead) may readahead several swap entries
> after the fault swap entry.  The readahead algorithm calculates some
> of the swap entries to readahead via increasing the offset of the
> fault swap entry without checking whether they are beyond the end of
> the swap device and it relys on the __swp_swapcount() and
> swapcache_prepare() to check it.  Although __swp_swapcount() checks
> for the swap entry passed in, it will complain with the error message
> as follow for the expected invalid swap entry.  This may make the end
> users confused.
> 
>   swap_info_get: Bad swap offset entry 0200f8a7
> 
> To fix the false error message, the swap entry checking is added in
> swap readahead to avoid to pass the out-bound swap entries and the
> swap entry reserved for the swap header to __swp_swapcount() and
> swapcache_prepare().

I have lost an overview in the swap code after recent changes so I
cannot really give you a responsible ack but this looks much better
than the previous attempt. So it looks good at first sight.

> Cc: Tim Chen 
> Cc: Minchan Kim 
> Cc: Michal Hocko 
> Cc:  # 4.11-4.13
> Reported-by: Christian Kujau 
> Fixes: e8c26ab60598 ("mm/swap: skip readahead for unreferenced swap slots")
> Signed-off-by: "Huang, Ying" 
> ---
>  include/linux/swap.h |  1 +
>  mm/swap_state.c  |  6 --
>  mm/swapfile.c| 21 +
>  3 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index 84255b3da7c1..43b4b821c805 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -476,6 +476,7 @@ extern int page_swapcount(struct page *);
>  extern int __swap_count(struct swap_info_struct *si, swp_entry_t entry);
>  extern int __swp_swapcount(swp_entry_t entry);
>  extern int swp_swapcount(swp_entry_t entry);
> +extern bool swap_entry_check(swp_entry_t entry);
>  extern struct swap_info_struct *page_swap_info(struct page *);
>  extern struct swap_info_struct *swp_swap_info(swp_entry_t entry);
>  extern bool reuse_swap_page(struct page *, int *);
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 6c017ced11e6..7dd70e77058d 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -569,11 +569,13 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t 
> gfp_mask,
>   /* Read a page_cluster sized and aligned cluster around offset. */
>   start_offset = offset & ~mask;
>   end_offset = offset | mask;
> - if (!start_offset)  /* First page is swap header. */
> - start_offset++;
>  
>   blk_start_plug();
>   for (offset = start_offset; offset <= end_offset ; offset++) {
> + swp_entry_t ent = swp_entry(swp_type(entry), offset);
> +
> + if (!swap_entry_check(ent))
> + continue;
>   /* Ok, do the async read-ahead now */
>   page = __read_swap_cache_async(
>   swp_entry(swp_type(entry), offset),
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 3074b02eaa09..b04cec29c234 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1107,6 +1107,27 @@ static struct swap_info_struct 
> *swap_info_get_cont(swp_entry_t entry,
>   return p;
>  }
>  
> +bool swap_entry_check(swp_entry_t entry)
> +{
> + struct swap_info_struct *p;
> + unsigned long offset, type;
> +
> + type = swp_type(entry);
> + if (type >= nr_swapfiles)
> + goto bad_file;
> + p = swap_info[type];
> + offset = swp_offset(entry);
> + if (unlikely(!offset || offset >= p->max))
> + goto out;
> +
> + return true;
> +
> +bad_file:
> + pr_err("%s: %s%08lx\n", __func__, Bad_file, entry.val);
> +out:
> + return false;
> +}
> +
>  static unsigned char __swap_entry_free(struct swap_info_struct *p,
>  swp_entry_t entry, unsigned char usage)
>  {
> -- 
> 2.14.2
> 

-- 
Michal Hocko
SUSE Labs

[PATCH -mm -V2] mm, swap: Fix false error message in __swp_swapcount()

[Huang, Ying]

From: Huang Ying 

When a page fault occurs for a swap entry, the physical swap readahead
(not the VMA base swap readahead) may readahead several swap entries
after the fault swap entry.  The readahead algorithm calculates some
of the swap entries to readahead via increasing the offset of the
fault swap entry without checking whether they are beyond the end of
the swap device and it relys on the __swp_swapcount() and
swapcache_prepare() to check it.  Although __swp_swapcount() checks
for the swap entry passed in, it will complain with the error message
as follow for the expected invalid swap entry.  This may make the end
users confused.

  swap_info_get: Bad swap offset entry 0200f8a7

To fix the false error message, the swap entry checking is added in
swap readahead to avoid to pass the out-bound swap entries and the
swap entry reserved for the swap header to __swp_swapcount() and
swapcache_prepare().

Cc: Tim Chen 
Cc: Minchan Kim 
Cc: Michal Hocko 
Cc:  # 4.11-4.13
Reported-by: Christian Kujau 
Fixes: e8c26ab60598 ("mm/swap: skip readahead for unreferenced swap slots")
Signed-off-by: "Huang, Ying" 
---
 include/linux/swap.h |  1 +
 mm/swap_state.c  |  6 --
 mm/swapfile.c| 21 +
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/include/linux/swap.h b/include/linux/swap.h
index 84255b3da7c1..43b4b821c805 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -476,6 +476,7 @@ extern int page_swapcount(struct page *);
 extern int __swap_count(struct swap_info_struct *si, swp_entry_t entry);
 extern int __swp_swapcount(swp_entry_t entry);
 extern int swp_swapcount(swp_entry_t entry);
+extern bool swap_entry_check(swp_entry_t entry);
 extern struct swap_info_struct *page_swap_info(struct page *);
 extern struct swap_info_struct *swp_swap_info(swp_entry_t entry);
 extern bool reuse_swap_page(struct page *, int *);
diff --git a/mm/swap_state.c b/mm/swap_state.c
index 6c017ced11e6..7dd70e77058d 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -569,11 +569,13 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t 
gfp_mask,
/* Read a page_cluster sized and aligned cluster around offset. */
start_offset = offset & ~mask;
end_offset = offset | mask;
-   if (!start_offset)  /* First page is swap header. */
-   start_offset++;
 
blk_start_plug();
for (offset = start_offset; offset <= end_offset ; offset++) {
+   swp_entry_t ent = swp_entry(swp_type(entry), offset);
+
+   if (!swap_entry_check(ent))
+   continue;
/* Ok, do the async read-ahead now */
page = __read_swap_cache_async(
swp_entry(swp_type(entry), offset),
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 3074b02eaa09..b04cec29c234 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1107,6 +1107,27 @@ static struct swap_info_struct 
*swap_info_get_cont(swp_entry_t entry,
return p;
 }
 
+bool swap_entry_check(swp_entry_t entry)
+{
+   struct swap_info_struct *p;
+   unsigned long offset, type;
+
+   type = swp_type(entry);
+   if (type >= nr_swapfiles)
+   goto bad_file;
+   p = swap_info[type];
+   offset = swp_offset(entry);
+   if (unlikely(!offset || offset >= p->max))
+   goto out;
+
+   return true;
+
+bad_file:
+   pr_err("%s: %s%08lx\n", __func__, Bad_file, entry.val);
+out:
+   return false;
+}
+
 static unsigned char __swap_entry_free(struct swap_info_struct *p,
   swp_entry_t entry, unsigned char usage)
 {
-- 
2.14.2

[PATCH -mm -V2] mm, swap: Fix false error message in __swp_swapcount()

[Huang, Ying]

From: Huang Ying 

When a page fault occurs for a swap entry, the physical swap readahead
(not the VMA base swap readahead) may readahead several swap entries
after the fault swap entry.  The readahead algorithm calculates some
of the swap entries to readahead via increasing the offset of the
fault swap entry without checking whether they are beyond the end of
the swap device and it relys on the __swp_swapcount() and
swapcache_prepare() to check it.  Although __swp_swapcount() checks
for the swap entry passed in, it will complain with the error message
as follow for the expected invalid swap entry.  This may make the end
users confused.

  swap_info_get: Bad swap offset entry 0200f8a7

To fix the false error message, the swap entry checking is added in
swap readahead to avoid to pass the out-bound swap entries and the
swap entry reserved for the swap header to __swp_swapcount() and
swapcache_prepare().

Cc: Tim Chen 
Cc: Minchan Kim 
Cc: Michal Hocko 
Cc:  # 4.11-4.13
Reported-by: Christian Kujau 
Fixes: e8c26ab60598 ("mm/swap: skip readahead for unreferenced swap slots")
Signed-off-by: "Huang, Ying" 
---
 include/linux/swap.h |  1 +
 mm/swap_state.c  |  6 --
 mm/swapfile.c| 21 +
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/include/linux/swap.h b/include/linux/swap.h
index 84255b3da7c1..43b4b821c805 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -476,6 +476,7 @@ extern int page_swapcount(struct page *);
 extern int __swap_count(struct swap_info_struct *si, swp_entry_t entry);
 extern int __swp_swapcount(swp_entry_t entry);
 extern int swp_swapcount(swp_entry_t entry);
+extern bool swap_entry_check(swp_entry_t entry);
 extern struct swap_info_struct *page_swap_info(struct page *);
 extern struct swap_info_struct *swp_swap_info(swp_entry_t entry);
 extern bool reuse_swap_page(struct page *, int *);
diff --git a/mm/swap_state.c b/mm/swap_state.c
index 6c017ced11e6..7dd70e77058d 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -569,11 +569,13 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t 
gfp_mask,
/* Read a page_cluster sized and aligned cluster around offset. */
start_offset = offset & ~mask;
end_offset = offset | mask;
-   if (!start_offset)  /* First page is swap header. */
-   start_offset++;
 
blk_start_plug();
for (offset = start_offset; offset <= end_offset ; offset++) {
+   swp_entry_t ent = swp_entry(swp_type(entry), offset);
+
+   if (!swap_entry_check(ent))
+   continue;
/* Ok, do the async read-ahead now */
page = __read_swap_cache_async(
swp_entry(swp_type(entry), offset),
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 3074b02eaa09..b04cec29c234 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1107,6 +1107,27 @@ static struct swap_info_struct 
*swap_info_get_cont(swp_entry_t entry,
return p;
 }
 
+bool swap_entry_check(swp_entry_t entry)
+{
+   struct swap_info_struct *p;
+   unsigned long offset, type;
+
+   type = swp_type(entry);
+   if (type >= nr_swapfiles)
+   goto bad_file;
+   p = swap_info[type];
+   offset = swp_offset(entry);
+   if (unlikely(!offset || offset >= p->max))
+   goto out;
+
+   return true;
+
+bad_file:
+   pr_err("%s: %s%08lx\n", __func__, Bad_file, entry.val);
+out:
+   return false;
+}
+
 static unsigned char __swap_entry_free(struct swap_info_struct *p,
   swp_entry_t entry, unsigned char usage)
 {
-- 
2.14.2