Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1295255 > unrolled thread

[PATCH] mm, oom: initiallize all new zap_details fields before use

Back to article view | Back to linux.kernel

Contents

  [PATCH] mm, oom: initiallize all new zap_details fields before use Sasha Levin <sasha.levin@oracle.com> - 2015-12-19 02:10 +0100
    Re: [PATCH] mm, oom: initiallize all new zap_details fields before  use "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-12-19 21:00 +0100
      Re: [PATCH] mm, oom: initiallize all new zap_details fields before  use Sasha Levin <sasha.levin@oracle.com> - 2015-12-19 23:10 +0100
        Re: [PATCH] mm, oom: initiallize all new zap_details fields before  use Andrew Morton <akpm@linux-foundation.org> - 2015-12-21 23:30 +0100
          Re: [PATCH] mm, oom: initiallize all new zap_details fields before  use Sasha Levin <sasha.levin@oracle.com> - 2015-12-22 02:00 +0100
    Re: [PATCH] mm, oom: initiallize all new zap_details fields before  use Michal Hocko <mhocko@kernel.org> - 2015-12-21 09:40 +0100

#1295255 — [PATCH] mm, oom: initiallize all new zap_details fields before use

Commit "mm, oom: introduce oom reaper" forgot to initialize the two new fields
of struct zap_details in unmap_mapping_range(). This caused using stack garbage
on the call to unmap_mapping_range_tree().

Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
 mm/memory.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/memory.c b/mm/memory.c
index 206c8cd..0e32993 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2431,6 +2431,7 @@ void unmap_mapping_range(struct address_space *mapping,
 	details.last_index = hba + hlen - 1;
 	if (details.last_index < details.first_index)
 		details.last_index = ULONG_MAX;
+	details.check_swap_entries = details.ignore_dirty = false;
 
 
 	/* DAX uses i_mmap_lock to serialise file truncate vs page fault */
-- 
1.7.10.4

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

[toc] | [next] | [standalone]

#1295440 — Re: [PATCH] mm, oom: initiallize all new zap_details fields before use

On Fri, Dec 18, 2015 at 08:04:51PM -0500, Sasha Levin wrote:
> Commit "mm, oom: introduce oom reaper" forgot to initialize the two new fields
> of struct zap_details in unmap_mapping_range(). This caused using stack garbage
> on the call to unmap_mapping_range_tree().
> 
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> ---
>  mm/memory.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 206c8cd..0e32993 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -2431,6 +2431,7 @@ void unmap_mapping_range(struct address_space *mapping,
>  	details.last_index = hba + hlen - 1;
>  	if (details.last_index < details.first_index)
>  		details.last_index = ULONG_MAX;
> +	details.check_swap_entries = details.ignore_dirty = false;

Should we use c99 initializer instead to make it future-proof?

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

[toc] | [prev] | [next] | [standalone]

#1295479 — Re: [PATCH] mm, oom: initiallize all new zap_details fields before use

On 12/19/2015 02:52 PM, Kirill A. Shutemov wrote:
> On Fri, Dec 18, 2015 at 08:04:51PM -0500, Sasha Levin wrote:
>> > Commit "mm, oom: introduce oom reaper" forgot to initialize the two new fields
>> > of struct zap_details in unmap_mapping_range(). This caused using stack garbage
>> > on the call to unmap_mapping_range_tree().
>> > 
>> > Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
>> > ---
>> >  mm/memory.c |    1 +
>> >  1 file changed, 1 insertion(+)
>> > 
>> > diff --git a/mm/memory.c b/mm/memory.c
>> > index 206c8cd..0e32993 100644
>> > --- a/mm/memory.c
>> > +++ b/mm/memory.c
>> > @@ -2431,6 +2431,7 @@ void unmap_mapping_range(struct address_space *mapping,
>> >  	details.last_index = hba + hlen - 1;
>> >  	if (details.last_index < details.first_index)
>> >  		details.last_index = ULONG_MAX;
>> > +	details.check_swap_entries = details.ignore_dirty = false;
> Should we use c99 initializer instead to make it future-proof?

I didn't do that to make these sort of failures obvious. In this case, if we would have
used an initializer and it would default to the "wrong" values it would be much harder
to find this bug.


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

[toc] | [prev] | [next] | [standalone]

#1296259 — Re: [PATCH] mm, oom: initiallize all new zap_details fields before use

On Sat, 19 Dec 2015 17:03:15 -0500 Sasha Levin <sasha.levin@oracle.com> wrote:

> On 12/19/2015 02:52 PM, Kirill A. Shutemov wrote:
> > On Fri, Dec 18, 2015 at 08:04:51PM -0500, Sasha Levin wrote:
> >> > Commit "mm, oom: introduce oom reaper" forgot to initialize the two new fields
> >> > of struct zap_details in unmap_mapping_range(). This caused using stack garbage
> >> > on the call to unmap_mapping_range_tree().
> >> > 
> >> > Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> >> > ---
> >> >  mm/memory.c |    1 +
> >> >  1 file changed, 1 insertion(+)
> >> > 
> >> > diff --git a/mm/memory.c b/mm/memory.c
> >> > index 206c8cd..0e32993 100644
> >> > --- a/mm/memory.c
> >> > +++ b/mm/memory.c
> >> > @@ -2431,6 +2431,7 @@ void unmap_mapping_range(struct address_space *mapping,
> >> >  	details.last_index = hba + hlen - 1;
> >> >  	if (details.last_index < details.first_index)
> >> >  		details.last_index = ULONG_MAX;
> >> > +	details.check_swap_entries = details.ignore_dirty = false;
> > Should we use c99 initializer instead to make it future-proof?
> 
> I didn't do that to make these sort of failures obvious. In this case, if we would have
> used an initializer and it would default to the "wrong" values it would be much harder
> to find this bug.
> 

If we're to make that approach useful and debuggable we should poison
the structure at the outset with some well-known and crazy pattern.  Or
use kasan.

But I don't think we need any special treatment here so yes, the
conventional way of zapping everything is best, IMO.

--- a/mm/memory.c~mm-oom-introduce-oom-reaper-fix-5-fix
+++ a/mm/memory.c
@@ -2414,7 +2414,7 @@ static inline void unmap_mapping_range_t
 void unmap_mapping_range(struct address_space *mapping,
 		loff_t const holebegin, loff_t const holelen, int even_cows)
 {
-	struct zap_details details;
+	struct zap_details details = { };
 	pgoff_t hba = holebegin >> PAGE_SHIFT;
 	pgoff_t hlen = (holelen + PAGE_SIZE - 1) >> PAGE_SHIFT;
 

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

[toc] | [prev] | [next] | [standalone]

#1296403 — Re: [PATCH] mm, oom: initiallize all new zap_details fields before use

On 12/21/2015 05:24 PM, Andrew Morton wrote:
>>> Should we use c99 initializer instead to make it future-proof?
>> > 
>> > I didn't do that to make these sort of failures obvious. In this case, if we would have
>> > used an initializer and it would default to the "wrong" values it would be much harder
>> > to find this bug.
>> > 
> If we're to make that approach useful and debuggable we should poison
> the structure at the outset with some well-known and crazy pattern.  Or
> use kasan.

We sort of do. Consider stack garbage as "poison"...

This bug was found using UBSan which complained that a bool suddenly had the
value of '64'.

If we go back to the scenario I've described, and the struct would have been
initialized on declaration, you'd have a much harder time finding it rather
than letting our existing and future tools find it.


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

[toc] | [prev] | [next] | [standalone]

#1295829 — Re: [PATCH] mm, oom: initiallize all new zap_details fields before use

On Fri 18-12-15 20:04:51, Sasha Levin wrote:
> Commit "mm, oom: introduce oom reaper" forgot to initialize the two new fields
> of struct zap_details in unmap_mapping_range(). This caused using stack garbage
> on the call to unmap_mapping_range_tree().
> 
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>

Thanks for catching that.
Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/memory.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 206c8cd..0e32993 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -2431,6 +2431,7 @@ void unmap_mapping_range(struct address_space *mapping,
>  	details.last_index = hba + hlen - 1;
>  	if (details.last_index < details.first_index)
>  		details.last_index = ULONG_MAX;
> +	details.check_swap_entries = details.ignore_dirty = false;
>  
>  
>  	/* DAX uses i_mmap_lock to serialise file truncate vs page fault */
> -- 
> 1.7.10.4
> 

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web