[PATCH net 1/1] xfrm: ipcomp: Free destination pages on acomp errors

[PATCH net 1/1] xfrm: ipcomp: Free destination pages on acomp errors

[Ren Wei]

From: Yilin Zhu <zylzyl2333@gmail.com>

ipcomp_setup_req() allocates destination pages for the acomp output
scatterlist. On successful completion, ipcomp_post_acomp() attaches the
used pages to the skb and frees any unused pages.

On an acomp error, ipcomp_post_acomp() skips directly to freeing the
request. acomp_request_free() only releases the request itself, so the
caller-allocated destination pages are left allocated.

Track the number of destination pages allocated for the request and free
them on the error path before releasing the request.

Fixes: eb2953d26971 ("xfrm: ipcomp: Use crypto_acomp interface")
Cc: stable@kernel.org
Reported-by: Yuan Tan <yuantan098@gmail.com>
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Reported-by: Xin Liu <bird@lzu.edu.cn>
Co-developed-by: Peihan Liu <ronbogo@outlook.com>
Signed-off-by: Peihan Liu <ronbogo@outlook.com>
Signed-off-by: Yilin Zhu <zylzyl2333@gmail.com>
Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
---
 net/xfrm/xfrm_ipcomp.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_ipcomp.c b/net/xfrm/xfrm_ipcomp.c
index 5f38dff16177..1e0080300ec7 100644
--- a/net/xfrm/xfrm_ipcomp.c
+++ b/net/xfrm/xfrm_ipcomp.c
@@ -33,6 +33,7 @@ struct ipcomp_data {
 
 struct ipcomp_req_extra {
 	struct xfrm_state *x;
+	int dpages;
 	struct scatterlist sg[];
 };
 
@@ -44,6 +45,12 @@ static inline struct ipcomp_skb_cb *ipcomp_cb(struct sk_buff *skb)
 	return cb;
 }
 
+static void ipcomp_free_dpages(struct scatterlist *dsg, int dpages)
+{
+	while (dpages--)
+		__free_page(sg_page(dsg++));
+}
+
 static int ipcomp_post_acomp(struct sk_buff *skb, int err, int hlen)
 {
 	struct acomp_req *req = ipcomp_cb(skb)->req;
@@ -51,8 +58,13 @@ static int ipcomp_post_acomp(struct sk_buff *skb, int err, int hlen)
 	struct scatterlist *dsg;
 	int len, dlen;
 
-	if (unlikely(err))
+	if (unlikely(err)) {
+		if (req) {
+			extra = acomp_request_extra(req);
+			ipcomp_free_dpages(extra->sg, extra->dpages);
+		}
 		goto out_free_req;
+	}
 
 	extra = acomp_request_extra(req);
 	dsg = extra->sg;
@@ -167,6 +179,7 @@ static struct acomp_req *ipcomp_setup_req(struct xfrm_state *x,
 
 	extra = acomp_request_extra(req);
 	extra->x = x;
+	extra->dpages = 0;
 
 	dsg = extra->sg;
 	sg = dsg + dnfrags;
@@ -184,6 +197,7 @@ static struct acomp_req *ipcomp_setup_req(struct xfrm_state *x,
 		if (!page)
 			break;
 		sg_set_page(dsg + i, page, PAGE_SIZE, 0);
+		extra->dpages++;
 		total += PAGE_SIZE;
 	}
 	if (!i)
-- 
2.47.3

[v2 PATCH] xfrm: ipcomp: Free destination pages on acomp errors

[Herbert Xu]

On Tue, May 05, 2026 at 04:52:59PM +0800, Ren Wei wrote:
> From: Yilin Zhu <zylzyl2333@gmail.com>
> 
> ipcomp_setup_req() allocates destination pages for the acomp output
> scatterlist. On successful completion, ipcomp_post_acomp() attaches the
> used pages to the skb and frees any unused pages.
> 
> On an acomp error, ipcomp_post_acomp() skips directly to freeing the
> request. acomp_request_free() only releases the request itself, so the
> caller-allocated destination pages are left allocated.
> 
> Track the number of destination pages allocated for the request and free
> them on the error path before releasing the request.
> 
> Fixes: eb2953d26971 ("xfrm: ipcomp: Use crypto_acomp interface")
> Cc: stable@kernel.org
> Reported-by: Yuan Tan <yuantan098@gmail.com>
> Reported-by: Yifan Wu <yifanwucs@gmail.com>
> Reported-by: Juefei Pu <tomapufckgml@gmail.com>
> Reported-by: Xin Liu <bird@lzu.edu.cn>
> Co-developed-by: Peihan Liu <ronbogo@outlook.com>
> Signed-off-by: Peihan Liu <ronbogo@outlook.com>
> Signed-off-by: Yilin Zhu <zylzyl2333@gmail.com>
> Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
> ---
>  net/xfrm/xfrm_ipcomp.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)

Thanks for the patch! How about just moving the out_free_req label:

---8<---
Move the out_free_req label up by a couple of lines so that the
allocated dst SG list gets freed on error as well as success.

Fixes: eb2953d26971 ("xfrm: ipcomp: Use crypto_acomp interface")
Cc: stable@kernel.org
Reported-by: Yuan Tan <yuantan098@gmail.com>
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Reported-by: Xin Liu <bird@lzu.edu.cn>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/xfrm/xfrm_ipcomp.c b/net/xfrm/xfrm_ipcomp.c
index 5f38dff16177..2947321b043d 100644
--- a/net/xfrm/xfrm_ipcomp.c
+++ b/net/xfrm/xfrm_ipcomp.c
@@ -51,11 +51,12 @@ static int ipcomp_post_acomp(struct sk_buff *skb, int err, int hlen)
 	struct scatterlist *dsg;
 	int len, dlen;
 
+	extra = acomp_request_extra(req);
+	dsg = extra->sg;
+
 	if (unlikely(err))
 		goto out_free_req;
 
-	extra = acomp_request_extra(req);
-	dsg = extra->sg;
 	dlen = req->dlen;
 
 	pskb_trim_unique(skb, 0);
@@ -84,10 +85,10 @@ static int ipcomp_post_acomp(struct sk_buff *skb, int err, int hlen)
 		skb_shinfo(skb)->nr_frags++;
 	} while ((dlen -= len));
 
+out_free_req:
 	for (; dsg; dsg = sg_next(dsg))
 		__free_page(sg_page(dsg));
 
-out_free_req:
 	acomp_request_free(req);
 	return err;
 }
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [v2 PATCH] xfrm: ipcomp: Free destination pages on acomp errors

[Orion Zhu]

On Tue, 5 May 2026 at 02:19, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Tue, May 05, 2026 at 04:52:59PM +0800, Ren Wei wrote:
> > From: Yilin Zhu <zylzyl2333@gmail.com>
> >
> > ipcomp_setup_req() allocates destination pages for the acomp output
> > scatterlist. On successful completion, ipcomp_post_acomp() attaches the
> > used pages to the skb and frees any unused pages.
> >
> > On an acomp error, ipcomp_post_acomp() skips directly to freeing the
> > request. acomp_request_free() only releases the request itself, so the
> > caller-allocated destination pages are left allocated.
> >
> > Track the number of destination pages allocated for the request and free
> > them on the error path before releasing the request.
> >
> > Fixes: eb2953d26971 ("xfrm: ipcomp: Use crypto_acomp interface")
> > Cc: stable@kernel.org
> > Reported-by: Yuan Tan <yuantan098@gmail.com>
> > Reported-by: Yifan Wu <yifanwucs@gmail.com>
> > Reported-by: Juefei Pu <tomapufckgml@gmail.com>
> > Reported-by: Xin Liu <bird@lzu.edu.cn>
> > Co-developed-by: Peihan Liu <ronbogo@outlook.com>
> > Signed-off-by: Peihan Liu <ronbogo@outlook.com>
> > Signed-off-by: Yilin Zhu <zylzyl2333@gmail.com>
> > Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
> > ---
> >  net/xfrm/xfrm_ipcomp.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
>
> Thanks for the patch! How about just moving the out_free_req label:
>
> ---8<---
> Move the out_free_req label up by a couple of lines so that the
> allocated dst SG list gets freed on error as well as success.
>
> Fixes: eb2953d26971 ("xfrm: ipcomp: Use crypto_acomp interface")
> Cc: stable@kernel.org
> Reported-by: Yuan Tan <yuantan098@gmail.com>
> Reported-by: Yifan Wu <yifanwucs@gmail.com>
> Reported-by: Juefei Pu <tomapufckgml@gmail.com>
> Reported-by: Xin Liu <bird@lzu.edu.cn>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> diff --git a/net/xfrm/xfrm_ipcomp.c b/net/xfrm/xfrm_ipcomp.c
> index 5f38dff16177..2947321b043d 100644
> --- a/net/xfrm/xfrm_ipcomp.c
> +++ b/net/xfrm/xfrm_ipcomp.c
> @@ -51,11 +51,12 @@ static int ipcomp_post_acomp(struct sk_buff *skb, int err, int hlen)
>         struct scatterlist *dsg;
>         int len, dlen;
>
> +       extra = acomp_request_extra(req);
> +       dsg = extra->sg;
> +
>         if (unlikely(err))
>                 goto out_free_req;
>
> -       extra = acomp_request_extra(req);
> -       dsg = extra->sg;
>         dlen = req->dlen;
>
>         pskb_trim_unique(skb, 0);
> @@ -84,10 +85,10 @@ static int ipcomp_post_acomp(struct sk_buff *skb, int err, int hlen)
>                 skb_shinfo(skb)->nr_frags++;
>         } while ((dlen -= len));
>
> +out_free_req:
>         for (; dsg; dsg = sg_next(dsg))
>                 __free_page(sg_page(dsg));
>
> -out_free_req:
>         acomp_request_free(req);
>         return err;
>  }
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Thanks, but I think moving the label is not safe for all paths reaching
ipcomp_post_acomp().

ipcomp_post_acomp() is also called when ipcomp_setup_req() returns an
ERR_PTR. In particular, acomp_request_alloc_extra() can fail after
ipcomp_cb(skb)->req is set to NULL, so moving acomp_request_extra(req)
before the error check would dereference NULL.

There are also setup failures after the request is allocated but before
the destination SG list contains any allocated pages. skb_to_sgvec() can
fail before sg_init_table(dsg, dnfrags), and alloc_page() can fail before
the first destination page is installed. In those cases walking extra->sg
with sg_next() and freeing sg_page(dsg) would operate on an uninitialized
or empty destination SG list, potentially freeing NULL or non-owned pages.

That is why the patch tracks the number of destination pages actually
allocated and frees exactly that count on the acomp error path.

Best regards,
Yilin Zhu

[v3 PATCH] xfrm: ipcomp: Free destination pages on acomp errors

[Herbert Xu]

Orion Zhu <zylzyl2333@gmail.com> wrote:
>
> Thanks, but I think moving the label is not safe for all paths reaching
> ipcomp_post_acomp().

Thanks for checking!

We could fix this by checking whether req is NULL and whether
sg_page(dsg) is NULL.

---8<---
Move the out_free_req label up by a couple of lines so that the
allocated dst SG list gets freed on error as well as success.

Fixes: eb2953d26971 ("xfrm: ipcomp: Use crypto_acomp interface")
Cc: stable@kernel.org
Reported-by: Yuan Tan <yuantan098@gmail.com>
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Reported-by: Xin Liu <bird@lzu.edu.cn>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/xfrm/xfrm_ipcomp.c b/net/xfrm/xfrm_ipcomp.c
index 5f38dff16177..671d48f8c937 100644
--- a/net/xfrm/xfrm_ipcomp.c
+++ b/net/xfrm/xfrm_ipcomp.c
@@ -51,11 +51,15 @@ static int ipcomp_post_acomp(struct sk_buff *skb, int err, int hlen)
 	struct scatterlist *dsg;
 	int len, dlen;
 
-	if (unlikely(err))
-		goto out_free_req;
+	if (unlikely(!req))
+		return err;
 
 	extra = acomp_request_extra(req);
 	dsg = extra->sg;
+
+	if (unlikely(err))
+		goto out_free_req;
+
 	dlen = req->dlen;
 
 	pskb_trim_unique(skb, 0);
@@ -84,10 +88,10 @@ static int ipcomp_post_acomp(struct sk_buff *skb, int err, int hlen)
 		skb_shinfo(skb)->nr_frags++;
 	} while ((dlen -= len));
 
-	for (; dsg; dsg = sg_next(dsg))
+out_free_req:
+	for (; dsg && sg_page(dsg); dsg = sg_next(dsg))
 		__free_page(sg_page(dsg));
 
-out_free_req:
 	acomp_request_free(req);
 	return err;
 }
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [v3 PATCH] xfrm: ipcomp: Free destination pages on acomp errors

[Yilin Zhu]

On Wed, 6 May 2026 at 06:24, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> Orion Zhu <zylzyl2333@gmail.com> wrote:
> >
> > Thanks, but I think moving the label is not safe for all paths reaching
> > ipcomp_post_acomp().
>
> Thanks for checking!
>
> We could fix this by checking whether req is NULL and whether
> sg_page(dsg) is NULL.
>
> ---8<---
> Move the out_free_req label up by a couple of lines so that the
> allocated dst SG list gets freed on error as well as success.
>
> Fixes: eb2953d26971 ("xfrm: ipcomp: Use crypto_acomp interface")
> Cc: stable@kernel.org
> Reported-by: Yuan Tan <yuantan098@gmail.com>
> Reported-by: Yifan Wu <yifanwucs@gmail.com>
> Reported-by: Juefei Pu <tomapufckgml@gmail.com>
> Reported-by: Xin Liu <bird@lzu.edu.cn>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> diff --git a/net/xfrm/xfrm_ipcomp.c b/net/xfrm/xfrm_ipcomp.c
> index 5f38dff16177..671d48f8c937 100644
> --- a/net/xfrm/xfrm_ipcomp.c
> +++ b/net/xfrm/xfrm_ipcomp.c
> @@ -51,11 +51,15 @@ static int ipcomp_post_acomp(struct sk_buff *skb, int err, int hlen)
>         struct scatterlist *dsg;
>         int len, dlen;
>
> -       if (unlikely(err))
> -               goto out_free_req;
> +       if (unlikely(!req))
> +               return err;
>
>         extra = acomp_request_extra(req);
>         dsg = extra->sg;
> +
> +       if (unlikely(err))
> +               goto out_free_req;
> +
>         dlen = req->dlen;
>
>         pskb_trim_unique(skb, 0);
> @@ -84,10 +88,10 @@ static int ipcomp_post_acomp(struct sk_buff *skb, int err, int hlen)
>                 skb_shinfo(skb)->nr_frags++;
>         } while ((dlen -= len));
>
> -       for (; dsg; dsg = sg_next(dsg))
> +out_free_req:
> +       for (; dsg && sg_page(dsg); dsg = sg_next(dsg))
>                 __free_page(sg_page(dsg));
>
> -out_free_req:
>         acomp_request_free(req);
>         return err;
>  }
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Hi Herbert,

Thanks for the update.

I think this version looks reasonable to me. Checking req before
dereferencing it and stopping the SG walk at a NULL sg_page() should
cover the setup failure paths I was concerned about, while still freeing
the allocated destination pages on the acomp error path.

Could you please also add my Reported-by tag?

Reported-by: Yilin Zhu <zylzyl2333@gmail.com>

Thanks,
Yilin

Re: [v3 PATCH] xfrm: ipcomp: Free destination pages on acomp errors

[Steffen Klassert]

On Wed, May 06, 2026 at 09:23:28PM +0800, Herbert Xu wrote:
> Orion Zhu <zylzyl2333@gmail.com> wrote:
> >
> > Thanks, but I think moving the label is not safe for all paths reaching
> > ipcomp_post_acomp().
> 
> Thanks for checking!
> 
> We could fix this by checking whether req is NULL and whether
> sg_page(dsg) is NULL.
> 
> ---8<---
> Move the out_free_req label up by a couple of lines so that the
> allocated dst SG list gets freed on error as well as success.
> 
> Fixes: eb2953d26971 ("xfrm: ipcomp: Use crypto_acomp interface")
> Cc: stable@kernel.org
> Reported-by: Yuan Tan <yuantan098@gmail.com>
> Reported-by: Yifan Wu <yifanwucs@gmail.com>
> Reported-by: Juefei Pu <tomapufckgml@gmail.com>
> Reported-by: Xin Liu <bird@lzu.edu.cn>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied, thanks a lot everyone!