[PATCH 0/2] xfs: kmalloc/kfree conversion fixes

[PATCH 0/2] xfs: kmalloc/kfree conversion fixes

[Dave Chinner]

Hi Chandan,

These two patches are fixes for the two recently reported issues
with the kmem removal patchset. They've passes fstests and fsmark
scalability tests with 64kB directory block size, should should have
been exercising >64kB allocations through xlog_kvmalloc(). These
should (eventuallly) trigger the vmalloc() path and so should
exercise the vfree path through kvfree() instead of crashing...

-Dave.

[PATCH 1/2] xfs: xfs_btree_bload_prep_block() should use __GFP_NOFAIL

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

This was missed in the conversion from KM* flags.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Fixes: 10634530f7ba ("xfs: convert kmem_zalloc() to kzalloc()")
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_btree_staging.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_btree_staging.c b/fs/xfs/libxfs/xfs_btree_staging.c
index f0c69f9bb169..e6ec01f25d90 100644
--- a/fs/xfs/libxfs/xfs_btree_staging.c
+++ b/fs/xfs/libxfs/xfs_btree_staging.c
@@ -406,7 +406,7 @@ xfs_btree_bload_prep_block(
 
 		/* Allocate a new incore btree root block. */
 		new_size = bbl->iroot_size(cur, level, nr_this_block, priv);
-		ifp->if_broot = kzalloc(new_size, GFP_KERNEL);
+		ifp->if_broot = kzalloc(new_size, GFP_KERNEL | __GFP_NOFAIL);
 		ifp->if_broot_bytes = (int)new_size;
 
 		/* Initialize it and send it out. */
-- 
2.43.0

[PATCH 2/2] xfs: use kvfree() in xlog_cil_free_logvec()

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The xfs_log_vec items are allocated by xlog_kvmalloc(), and so need
to be freed with kvfree. This was missed when coverting from the
kmem_free() API.

Reported-by: Chandan Babu R <chandanbabu@kernel.org>
Fixes: 49292576136f ("xfs: convert kmem_free() for kvmalloc users to kvfree()")
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log_cil.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index f15735d0296a..9544ddaef066 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -877,7 +877,7 @@ xlog_cil_free_logvec(
 	while (!list_empty(lv_chain)) {
 		lv = list_first_entry(lv_chain, struct xfs_log_vec, lv_list);
 		list_del_init(&lv->lv_list);
-		kfree(lv);
+		kvfree(lv);
 	}
 }
 
-- 
2.43.0

Re: [PATCH 2/2] xfs: use kvfree() in xlog_cil_free_logvec()

[Darrick J. Wong]

On Tue, Feb 27, 2024 at 11:05:32AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The xfs_log_vec items are allocated by xlog_kvmalloc(), and so need
> to be freed with kvfree. This was missed when coverting from the
> kmem_free() API.
> 
> Reported-by: Chandan Babu R <chandanbabu@kernel.org>
> Fixes: 49292576136f ("xfs: convert kmem_free() for kvmalloc users to kvfree()")
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_log_cil.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index f15735d0296a..9544ddaef066 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -877,7 +877,7 @@ xlog_cil_free_logvec(
>  	while (!list_empty(lv_chain)) {
>  		lv = list_first_entry(lv_chain, struct xfs_log_vec, lv_list);
>  		list_del_init(&lv->lv_list);
> -		kfree(lv);
> +		kvfree(lv);

Is it necessary to s/kfree/kvfree/ in xlog_cil_process_intents when we
free the xfs_log_vec that's attached to a xfs_log_item?

--D

>  	}
>  }
>  
> -- 
> 2.43.0
> 
> 

Re: [PATCH 1/2] xfs: xfs_btree_bload_prep_block() should use __GFP_NOFAIL

[Darrick J. Wong]

On Tue, Feb 27, 2024 at 11:05:31AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> This was missed in the conversion from KM* flags.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Fixes: 10634530f7ba ("xfs: convert kmem_zalloc() to kzalloc()")
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks correct to me,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_btree_staging.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_btree_staging.c b/fs/xfs/libxfs/xfs_btree_staging.c
> index f0c69f9bb169..e6ec01f25d90 100644
> --- a/fs/xfs/libxfs/xfs_btree_staging.c
> +++ b/fs/xfs/libxfs/xfs_btree_staging.c
> @@ -406,7 +406,7 @@ xfs_btree_bload_prep_block(
>  
>  		/* Allocate a new incore btree root block. */
>  		new_size = bbl->iroot_size(cur, level, nr_this_block, priv);
> -		ifp->if_broot = kzalloc(new_size, GFP_KERNEL);
> +		ifp->if_broot = kzalloc(new_size, GFP_KERNEL | __GFP_NOFAIL);
>  		ifp->if_broot_bytes = (int)new_size;
>  
>  		/* Initialize it and send it out. */
> -- 
> 2.43.0
> 
> 

Re: [PATCH 2/2] xfs: use kvfree() in xlog_cil_free_logvec()

[Dave Chinner]

On Mon, Feb 26, 2024 at 04:46:21PM -0800, Darrick J. Wong wrote:
> On Tue, Feb 27, 2024 at 11:05:32AM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > The xfs_log_vec items are allocated by xlog_kvmalloc(), and so need
> > to be freed with kvfree. This was missed when coverting from the
> > kmem_free() API.
> > 
> > Reported-by: Chandan Babu R <chandanbabu@kernel.org>
> > Fixes: 49292576136f ("xfs: convert kmem_free() for kvmalloc users to kvfree()")
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_log_cil.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> > index f15735d0296a..9544ddaef066 100644
> > --- a/fs/xfs/xfs_log_cil.c
> > +++ b/fs/xfs/xfs_log_cil.c
> > @@ -877,7 +877,7 @@ xlog_cil_free_logvec(
> >  	while (!list_empty(lv_chain)) {
> >  		lv = list_first_entry(lv_chain, struct xfs_log_vec, lv_list);
> >  		list_del_init(&lv->lv_list);
> > -		kfree(lv);
> > +		kvfree(lv);
> 
> Is it necessary to s/kfree/kvfree/ in xlog_cil_process_intents when we
> free the xfs_log_vec that's attached to a xfs_log_item?

Yes, it should, even though intents are pretty much
guaranteed to be small enough they will never use vmalloc.

Good catch.

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

[PATCH v2 2/2] xfs: use kvfree() in xlog_cil_free_logvec()

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The xfs_log_vec items are allocated by xlog_kvmalloc(), and so need
to be freed with kvfree. This was missed when coverting from the
kmem_free() API.

Reported-by: Chandan Babu R <chandanbabu@kernel.org>
Fixes: 49292576136f ("xfs: convert kmem_free() for kvmalloc users to kvfree()")
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---

Version 2:
- also fix kfree() in xlog_cil_process_intents().
- checked that kvfree() is used for all lip->li_lv_shadow freeing
  calls.

 fs/xfs/xfs_log_cil.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index f15735d0296a..4d52854bcb29 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -877,7 +877,7 @@ xlog_cil_free_logvec(
 	while (!list_empty(lv_chain)) {
 		lv = list_first_entry(lv_chain, struct xfs_log_vec, lv_list);
 		list_del_init(&lv->lv_list);
-		kfree(lv);
+		kvfree(lv);
 	}
 }
 
@@ -1717,7 +1717,7 @@ xlog_cil_process_intents(
 		set_bit(XFS_LI_WHITEOUT, &ilip->li_flags);
 		trace_xfs_cil_whiteout_mark(ilip);
 		len += ilip->li_lv->lv_bytes;
-		kfree(ilip->li_lv);
+		kvfree(ilip->li_lv);
 		ilip->li_lv = NULL;
 
 		xfs_trans_del_item(lip);

Re: [PATCH v2 2/2] xfs: use kvfree() in xlog_cil_free_logvec()

[Darrick J. Wong]

On Tue, Feb 27, 2024 at 02:01:26PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The xfs_log_vec items are allocated by xlog_kvmalloc(), and so need
> to be freed with kvfree. This was missed when coverting from the
> kmem_free() API.
> 
> Reported-by: Chandan Babu R <chandanbabu@kernel.org>
> Fixes: 49292576136f ("xfs: convert kmem_free() for kvmalloc users to kvfree()")
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks good to me, will run this one through fstestsclod overnight...
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
> 
> Version 2:
> - also fix kfree() in xlog_cil_process_intents().
> - checked that kvfree() is used for all lip->li_lv_shadow freeing
>   calls.
> 
>  fs/xfs/xfs_log_cil.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index f15735d0296a..4d52854bcb29 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -877,7 +877,7 @@ xlog_cil_free_logvec(
>  	while (!list_empty(lv_chain)) {
>  		lv = list_first_entry(lv_chain, struct xfs_log_vec, lv_list);
>  		list_del_init(&lv->lv_list);
> -		kfree(lv);
> +		kvfree(lv);
>  	}
>  }
>  
> @@ -1717,7 +1717,7 @@ xlog_cil_process_intents(
>  		set_bit(XFS_LI_WHITEOUT, &ilip->li_flags);
>  		trace_xfs_cil_whiteout_mark(ilip);
>  		len += ilip->li_lv->lv_bytes;
> -		kfree(ilip->li_lv);
> +		kvfree(ilip->li_lv);
>  		ilip->li_lv = NULL;
>  
>  		xfs_trans_del_item(lip);
> 

Re: [PATCH v2 2/2] xfs: use kvfree() in xlog_cil_free_logvec()

[Chandan Babu R]

On Tue, Feb 27, 2024 at 02:01:26 PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> The xfs_log_vec items are allocated by xlog_kvmalloc(), and so need
> to be freed with kvfree. This was missed when coverting from the
> kmem_free() API.
>
> Reported-by: Chandan Babu R <chandanbabu@kernel.org>

I have changed the Reported-by tag value to Darrick (since he reported it
first) when applying the patches to my local Git tree.

-- 
Chandan

Re: [PATCH 1/2] xfs: xfs_btree_bload_prep_block() should use __GFP_NOFAIL

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 2/2] xfs: use kvfree() in xlog_cil_free_logvec()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 2/2] xfs: use kvfree() in xlog_cil_free_logvec()

[Darrick J. Wong]

On Mon, Feb 26, 2024 at 08:25:37PM -0800, Darrick J. Wong wrote:
> On Tue, Feb 27, 2024 at 02:01:26PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > The xfs_log_vec items are allocated by xlog_kvmalloc(), and so need
> > to be freed with kvfree. This was missed when coverting from the
> > kmem_free() API.
> > 
> > Reported-by: Chandan Babu R <chandanbabu@kernel.org>
> > Fixes: 49292576136f ("xfs: convert kmem_free() for kvmalloc users to kvfree()")
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> 
> Looks good to me, will run this one through fstestsclod overnight...
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>

FWIW I didn't see any further crashes after applying this patch, so:
Tested-by: Darrick J. Wong <djwong@kernel.org>

--D

> --D
> 
> > ---
> > 
> > Version 2:
> > - also fix kfree() in xlog_cil_process_intents().
> > - checked that kvfree() is used for all lip->li_lv_shadow freeing
> >   calls.
> > 
> >  fs/xfs/xfs_log_cil.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> > index f15735d0296a..4d52854bcb29 100644
> > --- a/fs/xfs/xfs_log_cil.c
> > +++ b/fs/xfs/xfs_log_cil.c
> > @@ -877,7 +877,7 @@ xlog_cil_free_logvec(
> >  	while (!list_empty(lv_chain)) {
> >  		lv = list_first_entry(lv_chain, struct xfs_log_vec, lv_list);
> >  		list_del_init(&lv->lv_list);
> > -		kfree(lv);
> > +		kvfree(lv);
> >  	}
> >  }
> >  
> > @@ -1717,7 +1717,7 @@ xlog_cil_process_intents(
> >  		set_bit(XFS_LI_WHITEOUT, &ilip->li_flags);
> >  		trace_xfs_cil_whiteout_mark(ilip);
> >  		len += ilip->li_lv->lv_bytes;
> > -		kfree(ilip->li_lv);
> > +		kvfree(ilip->li_lv);
> >  		ilip->li_lv = NULL;
> >  
> >  		xfs_trans_del_item(lip);
> > 
>