[PATCH] xfs: assure zeroed memory buffers for certain kmem allocations

[PATCH] xfs: assure zeroed memory buffers for certain kmem allocations

[Bill O'Donnell]

Guarantee zeroed memory buffers for cases where potential memory
leak to disk can occur. In these cases, kmem_alloc is used and
doesn't zero the buffer, opening the possibility of information
leakage to disk.

Introduce a xfs_buf_flag, _XBF_KMZ, to indicate a request for a zeroed
buffer, and use existing infrastucture (xfs_buf_allocate_memory) to
obtain the already zeroed buffer from kernel memory.

This solution avoids the performance issue that would occur if a
wholesale change to replace kmem_alloc with kmem_zalloc was done.

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
 fs/xfs/xfs_buf.c | 8 ++++++--
 fs/xfs/xfs_buf.h | 4 +++-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 120ef99d09e8..916a3f782950 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -345,16 +345,19 @@ xfs_buf_allocate_memory(
 	unsigned short		page_count, i;
 	xfs_off_t		start, end;
 	int			error;
+	uint			kmflag_mask = 0;
 
 	/*
 	 * for buffers that are contained within a single page, just allocate
 	 * the memory from the heap - there's no need for the complexity of
 	 * page arrays to keep allocation down to order 0.
 	 */
+	if (flags & _XBF_KMZ)
+		kmflag_mask |= KM_ZERO;
 	size = BBTOB(bp->b_length);
 	if (size < PAGE_SIZE) {
 		int align_mask = xfs_buftarg_dma_alignment(bp->b_target);
-		bp->b_addr = kmem_alloc_io(size, align_mask, KM_NOFS);
+		bp->b_addr = kmem_alloc_io(size, align_mask, KM_NOFS | kmflag_mask);
 		if (!bp->b_addr) {
 			/* low memory - use alloc_page loop instead */
 			goto use_alloc_page;
@@ -391,7 +394,7 @@ xfs_buf_allocate_memory(
 		struct page	*page;
 		uint		retries = 0;
 retry:
-		page = alloc_page(gfp_mask);
+		page = alloc_page(gfp_mask | kmflag_mask);
 		if (unlikely(page == NULL)) {
 			if (flags & XBF_READ_AHEAD) {
 				bp->b_page_count = i;
@@ -683,6 +686,7 @@ xfs_buf_get_map(
 	struct xfs_buf		*new_bp;
 	int			error = 0;
 
+	flags |= _XBF_KMZ;
 	error = xfs_buf_find(target, map, nmaps, flags, NULL, &bp);
 
 	switch (error) {
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index f6ce17d8d848..416ff588240a 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -38,6 +38,7 @@
 #define _XBF_PAGES	 (1 << 20)/* backed by refcounted pages */
 #define _XBF_KMEM	 (1 << 21)/* backed by heap memory */
 #define _XBF_DELWRI_Q	 (1 << 22)/* buffer on a delwri queue */
+#define _XBF_KMZ	 (1 << 23)/* zeroed buffer required */
 
 typedef unsigned int xfs_buf_flags_t;
 
@@ -54,7 +55,8 @@ typedef unsigned int xfs_buf_flags_t;
 	{ XBF_UNMAPPED,		"UNMAPPED" },	/* ditto */\
 	{ _XBF_PAGES,		"PAGES" }, \
 	{ _XBF_KMEM,		"KMEM" }, \
-	{ _XBF_DELWRI_Q,	"DELWRI_Q" }
+	{ _XBF_DELWRI_Q,	"DELWRI_Q" }, \
+	{ _XBF_KMZ,             "KMEM_Z" }
 
 
 /*
-- 
2.21.0

Re: [PATCH] xfs: assure zeroed memory buffers for certain kmem allocations

[Eric Sandeen]

On 9/16/19 10:35 AM, Bill O'Donnell wrote:
> Guarantee zeroed memory buffers for cases where potential memory
> leak to disk can occur. In these cases, kmem_alloc is used and
> doesn't zero the buffer, opening the possibility of information
> leakage to disk.
> 
> Introduce a xfs_buf_flag, _XBF_KMZ, to indicate a request for a zeroed
> buffer, and use existing infrastucture (xfs_buf_allocate_memory) to
> obtain the already zeroed buffer from kernel memory.
> 
> This solution avoids the performance issue that would occur if a
> wholesale change to replace kmem_alloc with kmem_zalloc was done.
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>

I think this can probably be further optimized by not obtaining zeroed
memory when we're about to fill the buffer from disk as the very
next step.

(in this case, xfs_buf_read_map calls xfs_buf_get_map and then immediately
reads the buffer from disk with _xfs_buf_read)  xfs_buf_read_map adds
XBF_READ to the flags during this process.

So I wonder if this can be simplified/optimized by just checking for XBF_READ
in xfs_buf_allocate_memory's flags, and if it's not set, then request
zeroed memory, because that indicates a buffer we'll be filling in from
memory and subsequently writing to disk.

-Eric

> ---
>  fs/xfs/xfs_buf.c | 8 ++++++--
>  fs/xfs/xfs_buf.h | 4 +++-
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 120ef99d09e8..916a3f782950 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -345,16 +345,19 @@ xfs_buf_allocate_memory(
>  	unsigned short		page_count, i;
>  	xfs_off_t		start, end;
>  	int			error;
> +	uint			kmflag_mask = 0;
>  
>  	/*
>  	 * for buffers that are contained within a single page, just allocate
>  	 * the memory from the heap - there's no need for the complexity of
>  	 * page arrays to keep allocation down to order 0.
>  	 */
> +	if (flags & _XBF_KMZ)
> +		kmflag_mask |= KM_ZERO;
>  	size = BBTOB(bp->b_length);
>  	if (size < PAGE_SIZE) {
>  		int align_mask = xfs_buftarg_dma_alignment(bp->b_target);
> -		bp->b_addr = kmem_alloc_io(size, align_mask, KM_NOFS);
> +		bp->b_addr = kmem_alloc_io(size, align_mask, KM_NOFS | kmflag_mask);
>  		if (!bp->b_addr) {
>  			/* low memory - use alloc_page loop instead */
>  			goto use_alloc_page;
> @@ -391,7 +394,7 @@ xfs_buf_allocate_memory(
>  		struct page	*page;
>  		uint		retries = 0;
>  retry:
> -		page = alloc_page(gfp_mask);
> +		page = alloc_page(gfp_mask | kmflag_mask);
>  		if (unlikely(page == NULL)) {
>  			if (flags & XBF_READ_AHEAD) {
>  				bp->b_page_count = i;
> @@ -683,6 +686,7 @@ xfs_buf_get_map(
>  	struct xfs_buf		*new_bp;
>  	int			error = 0;
>  
> +	flags |= _XBF_KMZ;
>  	error = xfs_buf_find(target, map, nmaps, flags, NULL, &bp);
>  
>  	switch (error) {
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index f6ce17d8d848..416ff588240a 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -38,6 +38,7 @@
>  #define _XBF_PAGES	 (1 << 20)/* backed by refcounted pages */
>  #define _XBF_KMEM	 (1 << 21)/* backed by heap memory */
>  #define _XBF_DELWRI_Q	 (1 << 22)/* buffer on a delwri queue */
> +#define _XBF_KMZ	 (1 << 23)/* zeroed buffer required */
>  
>  typedef unsigned int xfs_buf_flags_t;
>  
> @@ -54,7 +55,8 @@ typedef unsigned int xfs_buf_flags_t;
>  	{ XBF_UNMAPPED,		"UNMAPPED" },	/* ditto */\
>  	{ _XBF_PAGES,		"PAGES" }, \
>  	{ _XBF_KMEM,		"KMEM" }, \
> -	{ _XBF_DELWRI_Q,	"DELWRI_Q" }
> +	{ _XBF_DELWRI_Q,	"DELWRI_Q" }, \
> +	{ _XBF_KMZ,             "KMEM_Z" }
>  
>  
>  /*
> 

Re: [PATCH] xfs: assure zeroed memory buffers for certain kmem allocations

[Darrick J. Wong]

On Mon, Sep 16, 2019 at 04:24:40PM -0500, Eric Sandeen wrote:
> On 9/16/19 10:35 AM, Bill O'Donnell wrote:
> > Guarantee zeroed memory buffers for cases where potential memory
> > leak to disk can occur. In these cases, kmem_alloc is used and
> > doesn't zero the buffer, opening the possibility of information
> > leakage to disk.
> > 
> > Introduce a xfs_buf_flag, _XBF_KMZ, to indicate a request for a zeroed
> > buffer, and use existing infrastucture (xfs_buf_allocate_memory) to
> > obtain the already zeroed buffer from kernel memory.
> > 
> > This solution avoids the performance issue that would occur if a
> > wholesale change to replace kmem_alloc with kmem_zalloc was done.
> > 
> > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> 
> I think this can probably be further optimized by not obtaining zeroed
> memory when we're about to fill the buffer from disk as the very
> next step.
> 
> (in this case, xfs_buf_read_map calls xfs_buf_get_map and then immediately
> reads the buffer from disk with _xfs_buf_read)  xfs_buf_read_map adds
> XBF_READ to the flags during this process.
> 
> So I wonder if this can be simplified/optimized by just checking for XBF_READ
> in xfs_buf_allocate_memory's flags, and if it's not set, then request
> zeroed memory, because that indicates a buffer we'll be filling in from
> memory and subsequently writing to disk.

I was wondering that ("Why can't we allocate a zeroed buffer only for
the get_buf case so that we don't have to do that for the read_buf
case?") too.  Once you do that then you can then remove all the explicit
memset calls too.

> -Eric
> 
> > ---
> >  fs/xfs/xfs_buf.c | 8 ++++++--
> >  fs/xfs/xfs_buf.h | 4 +++-
> >  2 files changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> > index 120ef99d09e8..916a3f782950 100644
> > --- a/fs/xfs/xfs_buf.c
> > +++ b/fs/xfs/xfs_buf.c
> > @@ -345,16 +345,19 @@ xfs_buf_allocate_memory(
> >  	unsigned short		page_count, i;
> >  	xfs_off_t		start, end;
> >  	int			error;
> > +	uint			kmflag_mask = 0;
> >  
> >  	/*
> >  	 * for buffers that are contained within a single page, just allocate
> >  	 * the memory from the heap - there's no need for the complexity of
> >  	 * page arrays to keep allocation down to order 0.
> >  	 */
> > +	if (flags & _XBF_KMZ)
> > +		kmflag_mask |= KM_ZERO;
> >  	size = BBTOB(bp->b_length);
> >  	if (size < PAGE_SIZE) {
> >  		int align_mask = xfs_buftarg_dma_alignment(bp->b_target);
> > -		bp->b_addr = kmem_alloc_io(size, align_mask, KM_NOFS);
> > +		bp->b_addr = kmem_alloc_io(size, align_mask, KM_NOFS | kmflag_mask);

Does this overflow 80 columns?

--D

> >  		if (!bp->b_addr) {
> >  			/* low memory - use alloc_page loop instead */
> >  			goto use_alloc_page;
> > @@ -391,7 +394,7 @@ xfs_buf_allocate_memory(
> >  		struct page	*page;
> >  		uint		retries = 0;
> >  retry:
> > -		page = alloc_page(gfp_mask);
> > +		page = alloc_page(gfp_mask | kmflag_mask);
> >  		if (unlikely(page == NULL)) {
> >  			if (flags & XBF_READ_AHEAD) {
> >  				bp->b_page_count = i;
> > @@ -683,6 +686,7 @@ xfs_buf_get_map(
> >  	struct xfs_buf		*new_bp;
> >  	int			error = 0;
> >  
> > +	flags |= _XBF_KMZ;
> >  	error = xfs_buf_find(target, map, nmaps, flags, NULL, &bp);
> >  
> >  	switch (error) {
> > diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> > index f6ce17d8d848..416ff588240a 100644
> > --- a/fs/xfs/xfs_buf.h
> > +++ b/fs/xfs/xfs_buf.h
> > @@ -38,6 +38,7 @@
> >  #define _XBF_PAGES	 (1 << 20)/* backed by refcounted pages */
> >  #define _XBF_KMEM	 (1 << 21)/* backed by heap memory */
> >  #define _XBF_DELWRI_Q	 (1 << 22)/* buffer on a delwri queue */
> > +#define _XBF_KMZ	 (1 << 23)/* zeroed buffer required */
> >  
> >  typedef unsigned int xfs_buf_flags_t;
> >  
> > @@ -54,7 +55,8 @@ typedef unsigned int xfs_buf_flags_t;
> >  	{ XBF_UNMAPPED,		"UNMAPPED" },	/* ditto */\
> >  	{ _XBF_PAGES,		"PAGES" }, \
> >  	{ _XBF_KMEM,		"KMEM" }, \
> > -	{ _XBF_DELWRI_Q,	"DELWRI_Q" }
> > +	{ _XBF_DELWRI_Q,	"DELWRI_Q" }, \
> > +	{ _XBF_KMZ,             "KMEM_Z" }
> >  
> >  
> >  /*
> > 

Re: [PATCH] xfs: assure zeroed memory buffers for certain kmem allocations

[Bill O'Donnell]

On Mon, Sep 16, 2019 at 04:24:40PM -0500, Eric Sandeen wrote:
> On 9/16/19 10:35 AM, Bill O'Donnell wrote:
> > Guarantee zeroed memory buffers for cases where potential memory
> > leak to disk can occur. In these cases, kmem_alloc is used and
> > doesn't zero the buffer, opening the possibility of information
> > leakage to disk.
> > 
> > Introduce a xfs_buf_flag, _XBF_KMZ, to indicate a request for a zeroed
> > buffer, and use existing infrastucture (xfs_buf_allocate_memory) to
> > obtain the already zeroed buffer from kernel memory.
> > 
> > This solution avoids the performance issue that would occur if a
> > wholesale change to replace kmem_alloc with kmem_zalloc was done.
> > 
> > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> 
> I think this can probably be further optimized by not obtaining zeroed
> memory when we're about to fill the buffer from disk as the very
> next step.

Yep. I missed that redundancy.

> 
> (in this case, xfs_buf_read_map calls xfs_buf_get_map and then immediately
> reads the buffer from disk with _xfs_buf_read)  xfs_buf_read_map adds
> XBF_READ to the flags during this process.
> 
> So I wonder if this can be simplified/optimized by just checking for XBF_READ
> in xfs_buf_allocate_memory's flags, and if it's not set, then request
> zeroed memory, because that indicates a buffer we'll be filling in from
> memory and subsequently writing to disk.

nod.

> 
> -Eric
> 
> > ---
> >  fs/xfs/xfs_buf.c | 8 ++++++--
> >  fs/xfs/xfs_buf.h | 4 +++-
> >  2 files changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> > index 120ef99d09e8..916a3f782950 100644
> > --- a/fs/xfs/xfs_buf.c
> > +++ b/fs/xfs/xfs_buf.c
> > @@ -345,16 +345,19 @@ xfs_buf_allocate_memory(
> >  	unsigned short		page_count, i;
> >  	xfs_off_t		start, end;
> >  	int			error;
> > +	uint			kmflag_mask = 0;
> >  
> >  	/*
> >  	 * for buffers that are contained within a single page, just allocate
> >  	 * the memory from the heap - there's no need for the complexity of
> >  	 * page arrays to keep allocation down to order 0.
> >  	 */
> > +	if (flags & _XBF_KMZ)
> > +		kmflag_mask |= KM_ZERO;
> >  	size = BBTOB(bp->b_length);
> >  	if (size < PAGE_SIZE) {
> >  		int align_mask = xfs_buftarg_dma_alignment(bp->b_target);
> > -		bp->b_addr = kmem_alloc_io(size, align_mask, KM_NOFS);
> > +		bp->b_addr = kmem_alloc_io(size, align_mask, KM_NOFS | kmflag_mask);
> >  		if (!bp->b_addr) {
> >  			/* low memory - use alloc_page loop instead */
> >  			goto use_alloc_page;
> > @@ -391,7 +394,7 @@ xfs_buf_allocate_memory(
> >  		struct page	*page;
> >  		uint		retries = 0;
> >  retry:
> > -		page = alloc_page(gfp_mask);
> > +		page = alloc_page(gfp_mask | kmflag_mask);
> >  		if (unlikely(page == NULL)) {
> >  			if (flags & XBF_READ_AHEAD) {
> >  				bp->b_page_count = i;
> > @@ -683,6 +686,7 @@ xfs_buf_get_map(
> >  	struct xfs_buf		*new_bp;
> >  	int			error = 0;
> >  
> > +	flags |= _XBF_KMZ;
> >  	error = xfs_buf_find(target, map, nmaps, flags, NULL, &bp);
> >  
> >  	switch (error) {
> > diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> > index f6ce17d8d848..416ff588240a 100644
> > --- a/fs/xfs/xfs_buf.h
> > +++ b/fs/xfs/xfs_buf.h
> > @@ -38,6 +38,7 @@
> >  #define _XBF_PAGES	 (1 << 20)/* backed by refcounted pages */
> >  #define _XBF_KMEM	 (1 << 21)/* backed by heap memory */
> >  #define _XBF_DELWRI_Q	 (1 << 22)/* buffer on a delwri queue */
> > +#define _XBF_KMZ	 (1 << 23)/* zeroed buffer required */
> >  
> >  typedef unsigned int xfs_buf_flags_t;
> >  
> > @@ -54,7 +55,8 @@ typedef unsigned int xfs_buf_flags_t;
> >  	{ XBF_UNMAPPED,		"UNMAPPED" },	/* ditto */\
> >  	{ _XBF_PAGES,		"PAGES" }, \
> >  	{ _XBF_KMEM,		"KMEM" }, \
> > -	{ _XBF_DELWRI_Q,	"DELWRI_Q" }
> > +	{ _XBF_DELWRI_Q,	"DELWRI_Q" }, \
> > +	{ _XBF_KMZ,             "KMEM_Z" }
> >  
> >  
> >  /*
> > 

Re: [PATCH] xfs: assure zeroed memory buffers for certain kmem allocations

[Dave Chinner]

On Mon, Sep 16, 2019 at 10:35:04AM -0500, Bill O'Donnell wrote:
> Guarantee zeroed memory buffers for cases where potential memory
> leak to disk can occur. In these cases, kmem_alloc is used and
> doesn't zero the buffer, opening the possibility of information
> leakage to disk.
> 
> Introduce a xfs_buf_flag, _XBF_KMZ, to indicate a request for a zeroed
> buffer, and use existing infrastucture (xfs_buf_allocate_memory) to
> obtain the already zeroed buffer from kernel memory.
> 
> This solution avoids the performance issue that would occur if a
> wholesale change to replace kmem_alloc with kmem_zalloc was done.
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
>  fs/xfs/xfs_buf.c | 8 ++++++--
>  fs/xfs/xfs_buf.h | 4 +++-
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 120ef99d09e8..916a3f782950 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -345,16 +345,19 @@ xfs_buf_allocate_memory(
>  	unsigned short		page_count, i;
>  	xfs_off_t		start, end;
>  	int			error;
> +	uint			kmflag_mask = 0;
>  
>  	/*
>  	 * for buffers that are contained within a single page, just allocate
>  	 * the memory from the heap - there's no need for the complexity of
>  	 * page arrays to keep allocation down to order 0.
>  	 */
> +	if (flags & _XBF_KMZ)
> +		kmflag_mask |= KM_ZERO;
>  	size = BBTOB(bp->b_length);
>  	if (size < PAGE_SIZE) {
>  		int align_mask = xfs_buftarg_dma_alignment(bp->b_target);
> -		bp->b_addr = kmem_alloc_io(size, align_mask, KM_NOFS);
> +		bp->b_addr = kmem_alloc_io(size, align_mask, KM_NOFS | kmflag_mask);
>  		if (!bp->b_addr) {
>  			/* low memory - use alloc_page loop instead */
>  			goto use_alloc_page;
> @@ -391,7 +394,7 @@ xfs_buf_allocate_memory(
>  		struct page	*page;
>  		uint		retries = 0;
>  retry:
> -		page = alloc_page(gfp_mask);
> +		page = alloc_page(gfp_mask | kmflag_mask);
>  		if (unlikely(page == NULL)) {
>  			if (flags & XBF_READ_AHEAD) {
>  				bp->b_page_count = i;
> @@ -683,6 +686,7 @@ xfs_buf_get_map(
>  	struct xfs_buf		*new_bp;
>  	int			error = 0;
>  
> +	flags |= _XBF_KMZ;
>  	error = xfs_buf_find(target, map, nmaps, flags, NULL, &bp);

IIRC, this flag was supposed to go into xfs_trans_get_buf_map()
and direct callers of xfs_buf_get*() that weren't in the read path.
That avoids the need for zeroing pages that we are going to DMA
actual data into before it gets to users...

>  	switch (error) {
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index f6ce17d8d848..416ff588240a 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -38,6 +38,7 @@
>  #define _XBF_PAGES	 (1 << 20)/* backed by refcounted pages */
>  #define _XBF_KMEM	 (1 << 21)/* backed by heap memory */
>  #define _XBF_DELWRI_Q	 (1 << 22)/* buffer on a delwri queue */
> +#define _XBF_KMZ	 (1 << 23)/* zeroed buffer required */

"KMZ" isn't very descriptive, and it shouldn't have a "_" prefix as
it's not internal to the buffer cache - it's a caller controlled
flag like XBF_TRYLOCK.

I'd suggest something like XBF_INIT_PAGES or XBF_ZERO to make it
clear we are asking for ithe buffer to be explicitly initialised
to zero.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH v2] xfs: assure zeroed memory buffers for certain kmem allocations

[Bill O'Donnell]

Guarantee zeroed memory buffers for cases where potential memory
leak to disk can occur. In these cases, kmem_alloc is used and
doesn't zero the buffer, opening the possibility of information
leakage to disk.

Introduce a xfs_buf_flag, XBF_ZERO, to indicate a request for a zeroed
buffer, and use existing infrastucture (xfs_buf_allocate_memory) to
obtain the already zeroed buffer from kernel memory.

This solution avoids the performance issue that would occur if a
wholesale change to replace kmem_alloc with kmem_zalloc was done.

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
v2: zeroed buffer not required for XBF_READ case. Correct placement
    and rename the XBF_ZERO flag.

 fs/xfs/xfs_buf.c       | 9 +++++++--
 fs/xfs/xfs_buf.h       | 2 ++
 fs/xfs/xfs_trans_buf.c | 2 ++
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 120ef99d09e8..0d96efff451e 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -345,6 +345,10 @@ xfs_buf_allocate_memory(
 	unsigned short		page_count, i;
 	xfs_off_t		start, end;
 	int			error;
+	uint			kmflag_mask = 0;
+
+	if ((flags & XBF_ZERO) && !(flags & XBF_READ))
+		kmflag_mask |= KM_ZERO;
 
 	/*
 	 * for buffers that are contained within a single page, just allocate
@@ -354,7 +358,8 @@ xfs_buf_allocate_memory(
 	size = BBTOB(bp->b_length);
 	if (size < PAGE_SIZE) {
 		int align_mask = xfs_buftarg_dma_alignment(bp->b_target);
-		bp->b_addr = kmem_alloc_io(size, align_mask, KM_NOFS);
+		bp->b_addr = kmem_alloc_io(size, align_mask,
+					   KM_NOFS | kmflag_mask);
 		if (!bp->b_addr) {
 			/* low memory - use alloc_page loop instead */
 			goto use_alloc_page;
@@ -391,7 +396,7 @@ xfs_buf_allocate_memory(
 		struct page	*page;
 		uint		retries = 0;
 retry:
-		page = alloc_page(gfp_mask);
+		page = alloc_page(gfp_mask | kmflag_mask);
 		if (unlikely(page == NULL)) {
 			if (flags & XBF_READ_AHEAD) {
 				bp->b_page_count = i;
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index f6ce17d8d848..dccdd653c9dc 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -33,6 +33,7 @@
 /* flags used only as arguments to access routines */
 #define XBF_TRYLOCK	 (1 << 16)/* lock requested, but do not wait */
 #define XBF_UNMAPPED	 (1 << 17)/* do not map the buffer */
+#define XBF_ZERO	 (1 << 18)/* zeroed buffer required */
 
 /* flags used only internally */
 #define _XBF_PAGES	 (1 << 20)/* backed by refcounted pages */
@@ -52,6 +53,7 @@ typedef unsigned int xfs_buf_flags_t;
 	{ XBF_WRITE_FAIL,	"WRITE_FAIL" }, \
 	{ XBF_TRYLOCK,		"TRYLOCK" },	/* should never be set */\
 	{ XBF_UNMAPPED,		"UNMAPPED" },	/* ditto */\
+	{ XBF_ZERO,		"KMEM_ZERO" }, \
 	{ _XBF_PAGES,		"PAGES" }, \
 	{ _XBF_KMEM,		"KMEM" }, \
 	{ _XBF_DELWRI_Q,	"DELWRI_Q" }
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index b5b3a78ef31c..087d413c1490 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -123,6 +123,8 @@ xfs_trans_get_buf_map(
 	xfs_buf_t		*bp;
 	struct xfs_buf_log_item	*bip;
 
+	flags |= XBF_ZERO;
+
 	if (!tp)
 		return xfs_buf_get_map(target, map, nmaps, flags);
 
-- 
2.21.0

Re: [PATCH v2] xfs: assure zeroed memory buffers for certain kmem allocations

[Darrick J. Wong]

On Wed, Sep 18, 2019 at 10:47:33AM -0500, Bill O'Donnell wrote:
> Guarantee zeroed memory buffers for cases where potential memory
> leak to disk can occur. In these cases, kmem_alloc is used and
> doesn't zero the buffer, opening the possibility of information
> leakage to disk.
> 
> Introduce a xfs_buf_flag, XBF_ZERO, to indicate a request for a zeroed
> buffer, and use existing infrastucture (xfs_buf_allocate_memory) to
> obtain the already zeroed buffer from kernel memory.
> 
> This solution avoids the performance issue that would occur if a
> wholesale change to replace kmem_alloc with kmem_zalloc was done.
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
> v2: zeroed buffer not required for XBF_READ case. Correct placement
>     and rename the XBF_ZERO flag.
> 
>  fs/xfs/xfs_buf.c       | 9 +++++++--
>  fs/xfs/xfs_buf.h       | 2 ++
>  fs/xfs/xfs_trans_buf.c | 2 ++
>  3 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 120ef99d09e8..0d96efff451e 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -345,6 +345,10 @@ xfs_buf_allocate_memory(
>  	unsigned short		page_count, i;
>  	xfs_off_t		start, end;
>  	int			error;
> +	uint			kmflag_mask = 0;
> +
> +	if ((flags & XBF_ZERO) && !(flags & XBF_READ))

The sole caller of xfs_buf_allocate_memory is xfs_buf_get_map.  If
_get_map is called from the *read_buf* functions, they pass in XBF_READ
to ensure buffer contents are read.  The other _get_map callers seem to
be initializing metadata blocks and do not set XBF_READ.

So I wonder, do you even need XBF_ZERO?  Or could this be reduced to:

	if (!(flags & XBF_READ))
		km_flag_mask |= KM_ZERO;

?

--D

> +		kmflag_mask |= KM_ZERO;
>  
>  	/*
>  	 * for buffers that are contained within a single page, just allocate
> @@ -354,7 +358,8 @@ xfs_buf_allocate_memory(
>  	size = BBTOB(bp->b_length);
>  	if (size < PAGE_SIZE) {
>  		int align_mask = xfs_buftarg_dma_alignment(bp->b_target);
> -		bp->b_addr = kmem_alloc_io(size, align_mask, KM_NOFS);
> +		bp->b_addr = kmem_alloc_io(size, align_mask,
> +					   KM_NOFS | kmflag_mask);
>  		if (!bp->b_addr) {
>  			/* low memory - use alloc_page loop instead */
>  			goto use_alloc_page;
> @@ -391,7 +396,7 @@ xfs_buf_allocate_memory(
>  		struct page	*page;
>  		uint		retries = 0;
>  retry:
> -		page = alloc_page(gfp_mask);
> +		page = alloc_page(gfp_mask | kmflag_mask);
>  		if (unlikely(page == NULL)) {
>  			if (flags & XBF_READ_AHEAD) {
>  				bp->b_page_count = i;
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index f6ce17d8d848..dccdd653c9dc 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -33,6 +33,7 @@
>  /* flags used only as arguments to access routines */
>  #define XBF_TRYLOCK	 (1 << 16)/* lock requested, but do not wait */
>  #define XBF_UNMAPPED	 (1 << 17)/* do not map the buffer */
> +#define XBF_ZERO	 (1 << 18)/* zeroed buffer required */
>  
>  /* flags used only internally */
>  #define _XBF_PAGES	 (1 << 20)/* backed by refcounted pages */
> @@ -52,6 +53,7 @@ typedef unsigned int xfs_buf_flags_t;
>  	{ XBF_WRITE_FAIL,	"WRITE_FAIL" }, \
>  	{ XBF_TRYLOCK,		"TRYLOCK" },	/* should never be set */\
>  	{ XBF_UNMAPPED,		"UNMAPPED" },	/* ditto */\
> +	{ XBF_ZERO,		"KMEM_ZERO" }, \
>  	{ _XBF_PAGES,		"PAGES" }, \
>  	{ _XBF_KMEM,		"KMEM" }, \
>  	{ _XBF_DELWRI_Q,	"DELWRI_Q" }
> diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> index b5b3a78ef31c..087d413c1490 100644
> --- a/fs/xfs/xfs_trans_buf.c
> +++ b/fs/xfs/xfs_trans_buf.c
> @@ -123,6 +123,8 @@ xfs_trans_get_buf_map(
>  	xfs_buf_t		*bp;
>  	struct xfs_buf_log_item	*bip;
>  
> +	flags |= XBF_ZERO;
> +
>  	if (!tp)
>  		return xfs_buf_get_map(target, map, nmaps, flags);
>  
> -- 
> 2.21.0
> 

Re: [PATCH v2] xfs: assure zeroed memory buffers for certain kmem allocations

[Bill O'Donnell]

On Wed, Sep 18, 2019 at 09:32:13AM -0700, Darrick J. Wong wrote:
> On Wed, Sep 18, 2019 at 10:47:33AM -0500, Bill O'Donnell wrote:
> > Guarantee zeroed memory buffers for cases where potential memory
> > leak to disk can occur. In these cases, kmem_alloc is used and
> > doesn't zero the buffer, opening the possibility of information
> > leakage to disk.
> > 
> > Introduce a xfs_buf_flag, XBF_ZERO, to indicate a request for a zeroed
> > buffer, and use existing infrastucture (xfs_buf_allocate_memory) to
> > obtain the already zeroed buffer from kernel memory.
> > 
> > This solution avoids the performance issue that would occur if a
> > wholesale change to replace kmem_alloc with kmem_zalloc was done.
> > 
> > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > ---
> > v2: zeroed buffer not required for XBF_READ case. Correct placement
> >     and rename the XBF_ZERO flag.
> > 
> >  fs/xfs/xfs_buf.c       | 9 +++++++--
> >  fs/xfs/xfs_buf.h       | 2 ++
> >  fs/xfs/xfs_trans_buf.c | 2 ++
> >  3 files changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> > index 120ef99d09e8..0d96efff451e 100644
> > --- a/fs/xfs/xfs_buf.c
> > +++ b/fs/xfs/xfs_buf.c
> > @@ -345,6 +345,10 @@ xfs_buf_allocate_memory(
> >  	unsigned short		page_count, i;
> >  	xfs_off_t		start, end;
> >  	int			error;
> > +	uint			kmflag_mask = 0;
> > +
> > +	if ((flags & XBF_ZERO) && !(flags & XBF_READ))
> 
> The sole caller of xfs_buf_allocate_memory is xfs_buf_get_map.  If
> _get_map is called from the *read_buf* functions, they pass in XBF_READ
> to ensure buffer contents are read.  The other _get_map callers seem to
> be initializing metadata blocks and do not set XBF_READ.
> 
> So I wonder, do you even need XBF_ZERO?  Or could this be reduced to:
> 
> 	if (!(flags & XBF_READ))
> 		km_flag_mask |= KM_ZERO;
> 
> ?
> 
> --D

Yeah, I see no cases of XBF_ZERO true AND XBF_READ true.
Anytime XBF_ZERO is true, XBF_READ is false.
So, yes, it could be reduced by eliminating XBF_ZERO.

Thanks,
Bill

> 
> > +		kmflag_mask |= KM_ZERO;
> >  
> >  	/*
> >  	 * for buffers that are contained within a single page, just allocate
> > @@ -354,7 +358,8 @@ xfs_buf_allocate_memory(
> >  	size = BBTOB(bp->b_length);
> >  	if (size < PAGE_SIZE) {
> >  		int align_mask = xfs_buftarg_dma_alignment(bp->b_target);
> > -		bp->b_addr = kmem_alloc_io(size, align_mask, KM_NOFS);
> > +		bp->b_addr = kmem_alloc_io(size, align_mask,
> > +					   KM_NOFS | kmflag_mask);
> >  		if (!bp->b_addr) {
> >  			/* low memory - use alloc_page loop instead */
> >  			goto use_alloc_page;
> > @@ -391,7 +396,7 @@ xfs_buf_allocate_memory(
> >  		struct page	*page;
> >  		uint		retries = 0;
> >  retry:
> > -		page = alloc_page(gfp_mask);
> > +		page = alloc_page(gfp_mask | kmflag_mask);
> >  		if (unlikely(page == NULL)) {
> >  			if (flags & XBF_READ_AHEAD) {
> >  				bp->b_page_count = i;
> > diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> > index f6ce17d8d848..dccdd653c9dc 100644
> > --- a/fs/xfs/xfs_buf.h
> > +++ b/fs/xfs/xfs_buf.h
> > @@ -33,6 +33,7 @@
> >  /* flags used only as arguments to access routines */
> >  #define XBF_TRYLOCK	 (1 << 16)/* lock requested, but do not wait */
> >  #define XBF_UNMAPPED	 (1 << 17)/* do not map the buffer */
> > +#define XBF_ZERO	 (1 << 18)/* zeroed buffer required */
> >  
> >  /* flags used only internally */
> >  #define _XBF_PAGES	 (1 << 20)/* backed by refcounted pages */
> > @@ -52,6 +53,7 @@ typedef unsigned int xfs_buf_flags_t;
> >  	{ XBF_WRITE_FAIL,	"WRITE_FAIL" }, \
> >  	{ XBF_TRYLOCK,		"TRYLOCK" },	/* should never be set */\
> >  	{ XBF_UNMAPPED,		"UNMAPPED" },	/* ditto */\
> > +	{ XBF_ZERO,		"KMEM_ZERO" }, \
> >  	{ _XBF_PAGES,		"PAGES" }, \
> >  	{ _XBF_KMEM,		"KMEM" }, \
> >  	{ _XBF_DELWRI_Q,	"DELWRI_Q" }
> > diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> > index b5b3a78ef31c..087d413c1490 100644
> > --- a/fs/xfs/xfs_trans_buf.c
> > +++ b/fs/xfs/xfs_trans_buf.c
> > @@ -123,6 +123,8 @@ xfs_trans_get_buf_map(
> >  	xfs_buf_t		*bp;
> >  	struct xfs_buf_log_item	*bip;
> >  
> > +	flags |= XBF_ZERO;
> > +
> >  	if (!tp)
> >  		return xfs_buf_get_map(target, map, nmaps, flags);
> >  
> > -- 
> > 2.21.0
> > 

[PATCH v3] xfs: assure zeroed memory buffers for certain kmem allocations

[Bill O'Donnell]

Guarantee zeroed memory buffers for cases where potential memory
leak to disk can occur. In these cases, kmem_alloc is used and
doesn't zero the buffer, opening the possibility of information
leakage to disk.

Use existing infrastucture (xfs_buf_allocate_memory) to obtain
the already zeroed buffer from kernel memory.

This solution avoids the performance issue that would occur if a
wholesale change to replace kmem_alloc with kmem_zalloc was done.

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
v3: remove XBF_ZERO flag, and instead use XBF_READ flag only.
v2: zeroed buffer not required for XBF_READ case. Correct placement
    and rename the XBF_ZERO flag.


fs/xfs/xfs_buf.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 120ef99d09e8..6fbe63f34a68 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -345,6 +345,10 @@ xfs_buf_allocate_memory(
 	unsigned short		page_count, i;
 	xfs_off_t		start, end;
 	int			error;
+	uint			kmflag_mask = 0;
+
+	if (!(flags & XBF_READ))
+		kmflag_mask |= KM_ZERO;
 
 	/*
 	 * for buffers that are contained within a single page, just allocate
@@ -354,7 +358,8 @@ xfs_buf_allocate_memory(
 	size = BBTOB(bp->b_length);
 	if (size < PAGE_SIZE) {
 		int align_mask = xfs_buftarg_dma_alignment(bp->b_target);
-		bp->b_addr = kmem_alloc_io(size, align_mask, KM_NOFS);
+		bp->b_addr = kmem_alloc_io(size, align_mask,
+					   KM_NOFS | kmflag_mask);
 		if (!bp->b_addr) {
 			/* low memory - use alloc_page loop instead */
 			goto use_alloc_page;
@@ -391,7 +396,7 @@ xfs_buf_allocate_memory(
 		struct page	*page;
 		uint		retries = 0;
 retry:
-		page = alloc_page(gfp_mask);
+		page = alloc_page(gfp_mask | kmflag_mask);
 		if (unlikely(page == NULL)) {
 			if (flags & XBF_READ_AHEAD) {
 				bp->b_page_count = i;
-- 
2.21.0

Re: [PATCH v3] xfs: assure zeroed memory buffers for certain kmem allocations

[Christoph Hellwig]

On Thu, Sep 19, 2019 at 10:01:54AM -0500, Bill O'Donnell wrote:
> +	uint			kmflag_mask = 0;
> +
> +	if (!(flags & XBF_READ))
> +		kmflag_mask |= KM_ZERO;

> @@ -391,7 +396,7 @@ xfs_buf_allocate_memory(
>  		struct page	*page;
>  		uint		retries = 0;
>  retry:
> -		page = alloc_page(gfp_mask);
> +		page = alloc_page(gfp_mask | kmflag_mask);

alloc_page takes GFP_ flags, not KM_.  In fact sparse should have warned
about this.

Re: [PATCH v3] xfs: assure zeroed memory buffers for certain kmem allocations

[Bill O'Donnell]

On Thu, Sep 19, 2019 at 10:03:53AM -0700, Christoph Hellwig wrote:
> On Thu, Sep 19, 2019 at 10:01:54AM -0500, Bill O'Donnell wrote:
> > +	uint			kmflag_mask = 0;
> > +
> > +	if (!(flags & XBF_READ))
> > +		kmflag_mask |= KM_ZERO;
> 
> > @@ -391,7 +396,7 @@ xfs_buf_allocate_memory(
> >  		struct page	*page;
> >  		uint		retries = 0;
> >  retry:
> > -		page = alloc_page(gfp_mask);
> > +		page = alloc_page(gfp_mask | kmflag_mask);
> 
> alloc_page takes GFP_ flags, not KM_.  In fact sparse should have warned
> about this.

I wondered if the KM flag needed conversion to GFP, but saw no warning.
Thanks-
Bill

Re: [PATCH v3] xfs: assure zeroed memory buffers for certain kmem allocations

[Christoph Hellwig]

On Thu, Sep 19, 2019 at 12:20:47PM -0500, Bill O'Donnell wrote:
> > > @@ -391,7 +396,7 @@ xfs_buf_allocate_memory(
> > >  		struct page	*page;
> > >  		uint		retries = 0;
> > >  retry:
> > > -		page = alloc_page(gfp_mask);
> > > +		page = alloc_page(gfp_mask | kmflag_mask);
> > 
> > alloc_page takes GFP_ flags, not KM_.  In fact sparse should have warned
> > about this.
> 
> I wondered if the KM flag needed conversion to GFP, but saw no warning.

I'd be tempted to just do a manual memset after either kind of
allocation.

[PATCH v4] xfs: assure zeroed memory buffers for certain kmem allocations

[Bill O'Donnell]

Guarantee zeroed memory buffers for cases where potential memory
leak to disk can occur. In these cases, kmem_alloc is used and
doesn't zero the buffer, opening the possibility of information
leakage to disk.

Use existing infrastucture (xfs_buf_allocate_memory) to obtain
the already zeroed buffer from kernel memory.

This solution avoids the performance issue that would occur if a
wholesale change to replace kmem_alloc with kmem_zalloc was done.

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
v4: use __GFP_ZERO as part of gfp_mask (instead of KM_ZERO)
v3: remove XBF_ZERO flag, and instead use XBF_READ flag only.
v2: zeroed buffer not required for XBF_READ case. Correct placement
    and rename the XBF_ZERO flag.

 fs/xfs/xfs_buf.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 120ef99d09e8..5d0a68de5fa6 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -345,6 +345,15 @@ xfs_buf_allocate_memory(
 	unsigned short		page_count, i;
 	xfs_off_t		start, end;
 	int			error;
+	uint			kmflag_mask = 0;
+
+	/*
+	 * assure zeroed buffer for non-read cases.
+	 */
+	if (!(flags & XBF_READ)) {
+		kmflag_mask |= KM_ZERO;
+		gfp_mask |= __GFP_ZERO;
+	}
 
 	/*
 	 * for buffers that are contained within a single page, just allocate
@@ -354,7 +363,8 @@ xfs_buf_allocate_memory(
 	size = BBTOB(bp->b_length);
 	if (size < PAGE_SIZE) {
 		int align_mask = xfs_buftarg_dma_alignment(bp->b_target);
-		bp->b_addr = kmem_alloc_io(size, align_mask, KM_NOFS);
+		bp->b_addr = kmem_alloc_io(size, align_mask,
+					   KM_NOFS | kmflag_mask);
 		if (!bp->b_addr) {
 			/* low memory - use alloc_page loop instead */
 			goto use_alloc_page;
-- 
2.21.0

Re: [PATCH v3] xfs: assure zeroed memory buffers for certain kmem allocations

[Eric Sandeen]

On 9/19/19 12:38 PM, Christoph Hellwig wrote:
> On Thu, Sep 19, 2019 at 12:20:47PM -0500, Bill O'Donnell wrote:
>>>> @@ -391,7 +396,7 @@ xfs_buf_allocate_memory(
>>>>  		struct page	*page;
>>>>  		uint		retries = 0;
>>>>  retry:
>>>> -		page = alloc_page(gfp_mask);
>>>> +		page = alloc_page(gfp_mask | kmflag_mask);
>>>
>>> alloc_page takes GFP_ flags, not KM_.  In fact sparse should have warned
>>> about this.
>>
>> I wondered if the KM flag needed conversion to GFP, but saw no warning.
> 
> I'd be tempted to just do a manual memset after either kind of
> allocation.

At some point I think Dave had suggested that at least when allocating pages,
using the flag would be more efficient?

-Eric

Re: [PATCH v3] xfs: assure zeroed memory buffers for certain kmem allocations

[Dave Chinner]

On Fri, Sep 20, 2019 at 09:59:41AM -0500, Eric Sandeen wrote:
> On 9/19/19 12:38 PM, Christoph Hellwig wrote:
> > On Thu, Sep 19, 2019 at 12:20:47PM -0500, Bill O'Donnell wrote:
> >>>> @@ -391,7 +396,7 @@ xfs_buf_allocate_memory(
> >>>>  		struct page	*page;
> >>>>  		uint		retries = 0;
> >>>>  retry:
> >>>> -		page = alloc_page(gfp_mask);
> >>>> +		page = alloc_page(gfp_mask | kmflag_mask);
> >>>
> >>> alloc_page takes GFP_ flags, not KM_.  In fact sparse should have warned
> >>> about this.
> >>
> >> I wondered if the KM flag needed conversion to GFP, but saw no warning.
> > 
> > I'd be tempted to just do a manual memset after either kind of
> > allocation.
> 
> At some point I think Dave had suggested that at least when allocating pages,
> using the flag would be more efficient?

With some configurations pages come from the free lists pre-zeroed,
and so don't need zeroing to initialise them (e.g. when memory
poisoning is turned on, or pages are being zeroed on free). Hence if
you use __GFP_ZERO the it will only zero if the page obtained from
the freelist isn't already zero. The __GFP_ZERO call will also use
the most efficient method of zeroing the page for the platform via
clear_page() rather than memset()....

/me shrugs and doesn't really care either way....

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v4] xfs: assure zeroed memory buffers for certain kmem allocations

[Darrick J. Wong]

On Thu, Sep 19, 2019 at 04:01:38PM -0500, Bill O'Donnell wrote:
> Guarantee zeroed memory buffers for cases where potential memory
> leak to disk can occur. In these cases, kmem_alloc is used and
> doesn't zero the buffer, opening the possibility of information
> leakage to disk.
> 
> Use existing infrastucture (xfs_buf_allocate_memory) to obtain
> the already zeroed buffer from kernel memory.
> 
> This solution avoids the performance issue that would occur if a
> wholesale change to replace kmem_alloc with kmem_zalloc was done.
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
> v4: use __GFP_ZERO as part of gfp_mask (instead of KM_ZERO)
> v3: remove XBF_ZERO flag, and instead use XBF_READ flag only.
> v2: zeroed buffer not required for XBF_READ case. Correct placement
>     and rename the XBF_ZERO flag.
> 
>  fs/xfs/xfs_buf.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 120ef99d09e8..5d0a68de5fa6 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -345,6 +345,15 @@ xfs_buf_allocate_memory(
>  	unsigned short		page_count, i;
>  	xfs_off_t		start, end;
>  	int			error;
> +	uint			kmflag_mask = 0;
> +
> +	/*
> +	 * assure zeroed buffer for non-read cases.
> +	 */
> +	if (!(flags & XBF_READ)) {
> +		kmflag_mask |= KM_ZERO;
> +		gfp_mask |= __GFP_ZERO;
> +	}

Jeez it feels grody to have to set two different flags variables just to
get __GFP_ZERO consistently but I'll run it through xfstests overnight.

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

>  
>  	/*
>  	 * for buffers that are contained within a single page, just allocate
> @@ -354,7 +363,8 @@ xfs_buf_allocate_memory(
>  	size = BBTOB(bp->b_length);
>  	if (size < PAGE_SIZE) {
>  		int align_mask = xfs_buftarg_dma_alignment(bp->b_target);
> -		bp->b_addr = kmem_alloc_io(size, align_mask, KM_NOFS);
> +		bp->b_addr = kmem_alloc_io(size, align_mask,
> +					   KM_NOFS | kmflag_mask);
>  		if (!bp->b_addr) {
>  			/* low memory - use alloc_page loop instead */
>  			goto use_alloc_page;
> -- 
> 2.21.0
> 

Re: [PATCH v4] xfs: assure zeroed memory buffers for certain kmem allocations

[Darrick J. Wong]

On Thu, Sep 19, 2019 at 04:01:38PM -0500, Bill O'Donnell wrote:
> Guarantee zeroed memory buffers for cases where potential memory
> leak to disk can occur. In these cases, kmem_alloc is used and
> doesn't zero the buffer, opening the possibility of information
> leakage to disk.
> 
> Use existing infrastucture (xfs_buf_allocate_memory) to obtain
> the already zeroed buffer from kernel memory.
> 
> This solution avoids the performance issue that would occur if a
> wholesale change to replace kmem_alloc with kmem_zalloc was done.
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
> v4: use __GFP_ZERO as part of gfp_mask (instead of KM_ZERO)
> v3: remove XBF_ZERO flag, and instead use XBF_READ flag only.
> v2: zeroed buffer not required for XBF_READ case. Correct placement
>     and rename the XBF_ZERO flag.
> 
>  fs/xfs/xfs_buf.c | 12 +++++++++++-

/me wakes up and wonders, what about the other kmem_alloc_io callers in
xfs?  I think we need to slip a KM_ZERO into the allocation call when we
allocate the log buffers, right?  What about log recovery?

--D

>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 120ef99d09e8..5d0a68de5fa6 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -345,6 +345,15 @@ xfs_buf_allocate_memory(
>  	unsigned short		page_count, i;
>  	xfs_off_t		start, end;
>  	int			error;
> +	uint			kmflag_mask = 0;
> +
> +	/*
> +	 * assure zeroed buffer for non-read cases.
> +	 */
> +	if (!(flags & XBF_READ)) {
> +		kmflag_mask |= KM_ZERO;
> +		gfp_mask |= __GFP_ZERO;
> +	}
>  
>  	/*
>  	 * for buffers that are contained within a single page, just allocate
> @@ -354,7 +363,8 @@ xfs_buf_allocate_memory(
>  	size = BBTOB(bp->b_length);
>  	if (size < PAGE_SIZE) {
>  		int align_mask = xfs_buftarg_dma_alignment(bp->b_target);
> -		bp->b_addr = kmem_alloc_io(size, align_mask, KM_NOFS);
> +		bp->b_addr = kmem_alloc_io(size, align_mask,
> +					   KM_NOFS | kmflag_mask);
>  		if (!bp->b_addr) {
>  			/* low memory - use alloc_page loop instead */
>  			goto use_alloc_page;
> -- 
> 2.21.0
>