This patchset persists the zswap pool's per-CPU acomp_ctx resources to
last until the pool is destroyed. It then simplifies the per-CPU
acomp_ctx mutex locking in zswap_compress()/zswap_decompress().

Further, it consistently uses the same checks for valid
acomp_ctx->acomp/req in zswap procedures that allocate/deallocate
acomp_ctx members.

These are independent submissions of patches 23 and 24 from [1], to
facilitate merging. The Acks are preserved from [1].

[1]: https://patchwork.kernel.org/project/linux-mm/list/?series=1046677

Kanchana P. Sridhar (2):
  mm: zswap: Tie per-CPU acomp_ctx lifetime to the pool.
  mm: zswap: Consistently use IS_ERR_OR_NULL() to check acomp_ctx
    resources.

 mm/zswap.c | 163 ++++++++++++++++++++++-------------------------------
 1 file changed, 66 insertions(+), 97 deletions(-)

-- 
2.39.5

On Fri, 13 Mar 2026 22:16:30 -0700 "Kanchana P. Sridhar" <kanchanapsridhar2026@gmail.com> wrote:

> 
> This patchset persists the zswap pool's per-CPU acomp_ctx resources to
> last until the pool is destroyed. It then simplifies the per-CPU
> acomp_ctx mutex locking in zswap_compress()/zswap_decompress().
> 
> Further, it consistently uses the same checks for valid
> acomp_ctx->acomp/req in zswap procedures that allocate/deallocate
> acomp_ctx members.

Sashiko has questions:
https://sashiko.dev/#/patchset/20260314051632.17931-1-kanchanapsridhar2026%40gmail.com

On Sat, Mar 14, 2026 at 5:11 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri, 13 Mar 2026 22:16:30 -0700 "Kanchana P. Sridhar" <kanchanapsridhar2026@gmail.com> wrote:
>
> >
> > This patchset persists the zswap pool's per-CPU acomp_ctx resources to
> > last until the pool is destroyed. It then simplifies the per-CPU
> > acomp_ctx mutex locking in zswap_compress()/zswap_decompress().
> >
> > Further, it consistently uses the same checks for valid
> > acomp_ctx->acomp/req in zswap procedures that allocate/deallocate
> > acomp_ctx members.
>
> Sashiko has questions:
> https://sashiko.dev/#/patchset/20260314051632.17931-1-kanchanapsridhar2026%40gmail.com
>

Thanks Andrew, for sharing the questions raised by Sashiko. I have
provided some responses below:

> > @@ -786,7 +786,7 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
> > return ret;
> >
> > acomp_ctx->acomp = crypto_alloc_acomp_node(pool->tfm_name, 0, 0, cpu_to_node(cpu));
> > - if (IS_ERR(acomp_ctx->acomp)) {
> > + if (IS_ERR_OR_NULL(acomp_ctx->acomp)) {
> Does crypto_alloc_acomp_node() ever return NULL?
> Looking at the error handling just below this check, if this were to
> actually return NULL, PTR_ERR(NULL) evaluates to 0. This would cause
> the function to incorrectly return 0 (success) instead of an error code,
> hiding the allocation failure.

This is a good catch. Just to provide context, this patch was
introduced based on Yosry's earlier comments in [1].

[1]: https://patchwork.kernel.org/comment/26282128/

crypto_alloc_acomp_node() currently does not return NULL. However, it
could, in future.
Since the rest of zswap_cpu_comp_prepare() dereferences
acomp_ctx->acomp, it depends on whether we want to future-proof the
code to handle a possible eventuality of crypto_alloc_acomp_node()
returning NULL.

If the maintainers think future-proofing is beneficial, I would need
to handle the PTR_ERR(NULL) which would send a false success status.
If we don't think we need to handle a future NULL return from
crypto_alloc_acomp_node(), then I don't think this change is needed.
We could leave it as IS_ERR(acomp_ctx->acomp). I would like to get the
maintainers' inputs on how to proceed.

> > acomp_ctx->req = acomp_request_alloc(acomp_ctx->acomp);
> > - if (!acomp_ctx->req) {
> > + if (IS_ERR_OR_NULL(acomp_ctx->req)) {
> Is this change necessary for acomp_request_alloc()?
> This function strictly returns NULL on allocation failure, not an error
> pointer. Changing this to IS_ERR_OR_NULL() obscures the actual API contract
> without providing a functional benefit.

As of now, acomp_request_alloc() returns a valid "req" or NULL in case
of an error. Same question as above. The only benefit would be making
the code more robust to handle changes in the acomp API in future.

> > pr_err("could not alloc crypto acomp_request %s\n",
> > pool->tfm_name);
> > goto fail;
> Can this error path lead to a double-free or use-after-free?
> This isn't a bug introduced by this patch, but while reviewing the modified
> error path, it appears that when zswap_cpu_comp_prepare() fails and jumps
> to fail, acomp_ctx_dealloc() frees acomp_ctx->acomp and acomp_ctx->buffer
> but does not set those pointers to NULL.
> Because acomp_ctx is persistent per-CPU memory, if the CPU later comes back
> online, the early initialization check:
> if (acomp_ctx->acomp) {
> WARN_ON_ONCE(IS_ERR(acomp_ctx->acomp));
> return 0;
> }
> would see the dangling pointer, incorrectly return success, and leave the
> CPU with an uninitialized mutex and freed buffer.

Thanks again, good catch. I was thinking some more about this today,
and realized this could happen if let's say a batch of CPUs were
online at pool creation time, and the startup callback succeeds for
all of them. Now, if a CPU "x" is brought online after pool creation
succeeds, encounters an error in zswap_cpu_comp_prepare(), calls
acomp_ctx_dealloc(), then CPU "x"'s state rewinding to CPU_OFFLINE. If
the CPU "x" is again brought back online, the above use-after-free
would occur. I had missed this during my response to Yosry's v14
comment [2] yesterday, sorry about that.

I will set the acomp_ctx pointers to NULL, as Yosry suggested in [2].

[2]: https://patchwork.kernel.org/comment/26773983/

> Additionally, if the pool creation fails entirely, the fallback cleanup loop
> in zswap_pool_create() calls acomp_ctx_dealloc() again for all CPUs, which
> would cause a double-free on the CPU that initially failed.
> Should acomp_ctx_dealloc() clear these pointers after freeing them?

Yes, I agree, and will include a fix in v2 once I get the maintainers'
feedback on how to proceed on patch 2.

Thanks,
Kanchana

> > > @@ -786,7 +786,7 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
> > > return ret;
> > >
> > > acomp_ctx->acomp = crypto_alloc_acomp_node(pool->tfm_name, 0, 0, cpu_to_node(cpu));
> > > - if (IS_ERR(acomp_ctx->acomp)) {
> > > + if (IS_ERR_OR_NULL(acomp_ctx->acomp)) {
> > Does crypto_alloc_acomp_node() ever return NULL?
> > Looking at the error handling just below this check, if this were to
> > actually return NULL, PTR_ERR(NULL) evaluates to 0. This would cause
> > the function to incorrectly return 0 (success) instead of an error code,
> > hiding the allocation failure.
> 
> This is a good catch. Just to provide context, this patch was
> introduced based on Yosry's earlier comments in [1].
> 
> [1]: https://patchwork.kernel.org/comment/26282128/
> 
> crypto_alloc_acomp_node() currently does not return NULL. However, it
> could, in future.
> Since the rest of zswap_cpu_comp_prepare() dereferences
> acomp_ctx->acomp, it depends on whether we want to future-proof the
> code to handle a possible eventuality of crypto_alloc_acomp_node()
> returning NULL.

Hmm upon revisiting this, I think keeping this as IS_ERR() here is a
better documentation for the API, and the incossitency between this code
and acomp_ctx_dealloc() is arguably documenting that the function can
only return an ERR, but it can also be NULL-initialized by zswap.

> 
> If the maintainers think future-proofing is beneficial, I would need
> to handle the PTR_ERR(NULL) which would send a false success status.
> If we don't think we need to handle a future NULL return from
> crypto_alloc_acomp_node(), then I don't think this change is needed.
> We could leave it as IS_ERR(acomp_ctx->acomp). I would like to get the
> maintainers' inputs on how to proceed.
> 
> > > acomp_ctx->req = acomp_request_alloc(acomp_ctx->acomp);
> > > - if (!acomp_ctx->req) {
> > > + if (IS_ERR_OR_NULL(acomp_ctx->req)) {
> > Is this change necessary for acomp_request_alloc()?
> > This function strictly returns NULL on allocation failure, not an error
> > pointer. Changing this to IS_ERR_OR_NULL() obscures the actual API contract
> > without providing a functional benefit.
> 
> As of now, acomp_request_alloc() returns a valid "req" or NULL in case
> of an error. Same question as above. The only benefit would be making
> the code more robust to handle changes in the acomp API in future.

For this one, do we need to do IS_ERR_OR_NULL() in acomp_ctx_dealloc()
to begin with? If acomp_request_alloc() only returns NULL, maybe that
should also be a NULL check?

In this case, we don't really need to make any changes here, and I think
this patch can just be dropped.

On Mon, Mar 16, 2026 at 8:06 AM Yosry Ahmed <yosry@kernel.org> wrote:
>
> > > > @@ -786,7 +786,7 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
> > > > return ret;
> > > >
> > > > acomp_ctx->acomp = crypto_alloc_acomp_node(pool->tfm_name, 0, 0, cpu_to_node(cpu));
> > > > - if (IS_ERR(acomp_ctx->acomp)) {
> > > > + if (IS_ERR_OR_NULL(acomp_ctx->acomp)) {
> > > Does crypto_alloc_acomp_node() ever return NULL?
> > > Looking at the error handling just below this check, if this were to
> > > actually return NULL, PTR_ERR(NULL) evaluates to 0. This would cause
> > > the function to incorrectly return 0 (success) instead of an error code,
> > > hiding the allocation failure.
> >
> > This is a good catch. Just to provide context, this patch was
> > introduced based on Yosry's earlier comments in [1].
> >
> > [1]: https://patchwork.kernel.org/comment/26282128/
> >
> > crypto_alloc_acomp_node() currently does not return NULL. However, it
> > could, in future.
> > Since the rest of zswap_cpu_comp_prepare() dereferences
> > acomp_ctx->acomp, it depends on whether we want to future-proof the
> > code to handle a possible eventuality of crypto_alloc_acomp_node()
> > returning NULL.
>
> Hmm upon revisiting this, I think keeping this as IS_ERR() here is a
> better documentation for the API, and the incossitency between this code
> and acomp_ctx_dealloc() is arguably documenting that the function can
> only return an ERR, but it can also be NULL-initialized by zswap.

Yes, makes sense.

>
> >
> > If the maintainers think future-proofing is beneficial, I would need
> > to handle the PTR_ERR(NULL) which would send a false success status.
> > If we don't think we need to handle a future NULL return from
> > crypto_alloc_acomp_node(), then I don't think this change is needed.
> > We could leave it as IS_ERR(acomp_ctx->acomp). I would like to get the
> > maintainers' inputs on how to proceed.
> >
> > > > acomp_ctx->req = acomp_request_alloc(acomp_ctx->acomp);
> > > > - if (!acomp_ctx->req) {
> > > > + if (IS_ERR_OR_NULL(acomp_ctx->req)) {
> > > Is this change necessary for acomp_request_alloc()?
> > > This function strictly returns NULL on allocation failure, not an error
> > > pointer. Changing this to IS_ERR_OR_NULL() obscures the actual API contract
> > > without providing a functional benefit.
> >
> > As of now, acomp_request_alloc() returns a valid "req" or NULL in case
> > of an error. Same question as above. The only benefit would be making
> > the code more robust to handle changes in the acomp API in future.
>
> For this one, do we need to do IS_ERR_OR_NULL() in acomp_ctx_dealloc()
> to begin with? If acomp_request_alloc() only returns NULL, maybe that
> should also be a NULL check?

This one is debatable, since acomp_ctx_dealloc() is intended to
replace zswap_cpu_comp_dead(), which has the IS_ERR_OR_NULL(). I think
replacing this with IS_NULL(req) makes sense, but would like to
confirm with you if changing existing behavior is Ok.

>
> In this case, we don't really need to make any changes here, and I think
> this patch can just be dropped.

I agree.

Thanks,
Kanchana

> > > If the maintainers think future-proofing is beneficial, I would need
> > > to handle the PTR_ERR(NULL) which would send a false success status.
> > > If we don't think we need to handle a future NULL return from
> > > crypto_alloc_acomp_node(), then I don't think this change is needed.
> > > We could leave it as IS_ERR(acomp_ctx->acomp). I would like to get the
> > > maintainers' inputs on how to proceed.
> > >
> > > > > acomp_ctx->req = acomp_request_alloc(acomp_ctx->acomp);
> > > > > - if (!acomp_ctx->req) {
> > > > > + if (IS_ERR_OR_NULL(acomp_ctx->req)) {
> > > > Is this change necessary for acomp_request_alloc()?
> > > > This function strictly returns NULL on allocation failure, not an error
> > > > pointer. Changing this to IS_ERR_OR_NULL() obscures the actual API contract
> > > > without providing a functional benefit.
> > >
> > > As of now, acomp_request_alloc() returns a valid "req" or NULL in case
> > > of an error. Same question as above. The only benefit would be making
> > > the code more robust to handle changes in the acomp API in future.
> >
> > For this one, do we need to do IS_ERR_OR_NULL() in acomp_ctx_dealloc()
> > to begin with? If acomp_request_alloc() only returns NULL, maybe that
> > should also be a NULL check?
>
> This one is debatable, since acomp_ctx_dealloc() is intended to
> replace zswap_cpu_comp_dead(), which has the IS_ERR_OR_NULL(). I think
> replacing this with IS_NULL(req) makes sense, but would like to
> confirm with you if changing existing behavior is Ok.

I think it's fine as long as acomp_request_alloc() never returns an
error. Maybe do it in a separate patch first, change IS_ERR_OR_NULL()
to a NULL check in zswap_cpu_comp_dead(), with the reasoning explained
in the changelog, to avoid hiding that change within the bigger patch.

Actually looking at zswap_cpu_comp_dead(), is the IS_ERR_OR_NULL()
check on acomp_ctx also misleading? Should that also just be a NULL
check?

On Mon, Mar 16, 2026 at 11:30 AM Yosry Ahmed <yosry@kernel.org> wrote:
>
> > > > If the maintainers think future-proofing is beneficial, I would need
> > > > to handle the PTR_ERR(NULL) which would send a false success status.
> > > > If we don't think we need to handle a future NULL return from
> > > > crypto_alloc_acomp_node(), then I don't think this change is needed.
> > > > We could leave it as IS_ERR(acomp_ctx->acomp). I would like to get the
> > > > maintainers' inputs on how to proceed.
> > > >
> > > > > > acomp_ctx->req = acomp_request_alloc(acomp_ctx->acomp);
> > > > > > - if (!acomp_ctx->req) {
> > > > > > + if (IS_ERR_OR_NULL(acomp_ctx->req)) {
> > > > > Is this change necessary for acomp_request_alloc()?
> > > > > This function strictly returns NULL on allocation failure, not an error
> > > > > pointer. Changing this to IS_ERR_OR_NULL() obscures the actual API contract
> > > > > without providing a functional benefit.
> > > >
> > > > As of now, acomp_request_alloc() returns a valid "req" or NULL in case
> > > > of an error. Same question as above. The only benefit would be making
> > > > the code more robust to handle changes in the acomp API in future.
> > >
> > > For this one, do we need to do IS_ERR_OR_NULL() in acomp_ctx_dealloc()
> > > to begin with? If acomp_request_alloc() only returns NULL, maybe that
> > > should also be a NULL check?
> >
> > This one is debatable, since acomp_ctx_dealloc() is intended to
> > replace zswap_cpu_comp_dead(), which has the IS_ERR_OR_NULL(). I think
> > replacing this with IS_NULL(req) makes sense, but would like to
> > confirm with you if changing existing behavior is Ok.
>
> I think it's fine as long as acomp_request_alloc() never returns an
> error. Maybe do it in a separate patch first, change IS_ERR_OR_NULL()
> to a NULL check in zswap_cpu_comp_dead(), with the reasoning explained
> in the changelog, to avoid hiding that change within the bigger patch.

Sounds good.

>
> Actually looking at zswap_cpu_comp_dead(), is the IS_ERR_OR_NULL()
> check on acomp_ctx also misleading? Should that also just be a NULL
> check?

Even a NULL check would be redundant in this case, per my
understanding, because if the alloc_percpu() call in
zswap_pool_create() had failed, pool creation would have failed.

I think a NULL check on the acomp_ctx would still be a good idea, just
in case, since this is all part of CPU hotplug. I agree, we don't need
an IS_ERR() check on acomp_ctx.

Thanks,
Kanchana

On Mon, Mar 16, 2026 at 12:21 PM Kanchana P. Sridhar
<kanchanapsridhar2026@gmail.com> wrote:
>
> On Mon, Mar 16, 2026 at 11:30 AM Yosry Ahmed <yosry@kernel.org> wrote:
> >
> > > > > If the maintainers think future-proofing is beneficial, I would need
> > > > > to handle the PTR_ERR(NULL) which would send a false success status.
> > > > > If we don't think we need to handle a future NULL return from
> > > > > crypto_alloc_acomp_node(), then I don't think this change is needed.
> > > > > We could leave it as IS_ERR(acomp_ctx->acomp). I would like to get the
> > > > > maintainers' inputs on how to proceed.
> > > > >
> > > > > > > acomp_ctx->req = acomp_request_alloc(acomp_ctx->acomp);
> > > > > > > - if (!acomp_ctx->req) {
> > > > > > > + if (IS_ERR_OR_NULL(acomp_ctx->req)) {
> > > > > > Is this change necessary for acomp_request_alloc()?
> > > > > > This function strictly returns NULL on allocation failure, not an error
> > > > > > pointer. Changing this to IS_ERR_OR_NULL() obscures the actual API contract
> > > > > > without providing a functional benefit.
> > > > >
> > > > > As of now, acomp_request_alloc() returns a valid "req" or NULL in case
> > > > > of an error. Same question as above. The only benefit would be making
> > > > > the code more robust to handle changes in the acomp API in future.
> > > >
> > > > For this one, do we need to do IS_ERR_OR_NULL() in acomp_ctx_dealloc()
> > > > to begin with? If acomp_request_alloc() only returns NULL, maybe that
> > > > should also be a NULL check?
> > >
> > > This one is debatable, since acomp_ctx_dealloc() is intended to
> > > replace zswap_cpu_comp_dead(), which has the IS_ERR_OR_NULL(). I think
> > > replacing this with IS_NULL(req) makes sense, but would like to
> > > confirm with you if changing existing behavior is Ok.
> >
> > I think it's fine as long as acomp_request_alloc() never returns an
> > error. Maybe do it in a separate patch first, change IS_ERR_OR_NULL()
> > to a NULL check in zswap_cpu_comp_dead(), with the reasoning explained
> > in the changelog, to avoid hiding that change within the bigger patch.
>
> Sounds good.
>
> >
> > Actually looking at zswap_cpu_comp_dead(), is the IS_ERR_OR_NULL()
> > check on acomp_ctx also misleading? Should that also just be a NULL
> > check?
>
> Even a NULL check would be redundant in this case, per my
> understanding, because if the alloc_percpu() call in
> zswap_pool_create() had failed, pool creation would have failed.
>
> I think a NULL check on the acomp_ctx would still be a good idea, just
> in case, since this is all part of CPU hotplug. I agree, we don't need
> an IS_ERR() check on acomp_ctx.

So I think we do one patch to convert both IS_ERR_OR_NULL() to NULL
checks, and then the current patch 1, right?

On Mon, Mar 16, 2026 at 12:24 PM Yosry Ahmed <yosry@kernel.org> wrote:
>
> On Mon, Mar 16, 2026 at 12:21 PM Kanchana P. Sridhar
> <kanchanapsridhar2026@gmail.com> wrote:
> >
> > On Mon, Mar 16, 2026 at 11:30 AM Yosry Ahmed <yosry@kernel.org> wrote:
> > >
> > > > > > If the maintainers think future-proofing is beneficial, I would need
> > > > > > to handle the PTR_ERR(NULL) which would send a false success status.
> > > > > > If we don't think we need to handle a future NULL return from
> > > > > > crypto_alloc_acomp_node(), then I don't think this change is needed.
> > > > > > We could leave it as IS_ERR(acomp_ctx->acomp). I would like to get the
> > > > > > maintainers' inputs on how to proceed.
> > > > > >
> > > > > > > > acomp_ctx->req = acomp_request_alloc(acomp_ctx->acomp);
> > > > > > > > - if (!acomp_ctx->req) {
> > > > > > > > + if (IS_ERR_OR_NULL(acomp_ctx->req)) {
> > > > > > > Is this change necessary for acomp_request_alloc()?
> > > > > > > This function strictly returns NULL on allocation failure, not an error
> > > > > > > pointer. Changing this to IS_ERR_OR_NULL() obscures the actual API contract
> > > > > > > without providing a functional benefit.
> > > > > >
> > > > > > As of now, acomp_request_alloc() returns a valid "req" or NULL in case
> > > > > > of an error. Same question as above. The only benefit would be making
> > > > > > the code more robust to handle changes in the acomp API in future.
> > > > >
> > > > > For this one, do we need to do IS_ERR_OR_NULL() in acomp_ctx_dealloc()
> > > > > to begin with? If acomp_request_alloc() only returns NULL, maybe that
> > > > > should also be a NULL check?
> > > >
> > > > This one is debatable, since acomp_ctx_dealloc() is intended to
> > > > replace zswap_cpu_comp_dead(), which has the IS_ERR_OR_NULL(). I think
> > > > replacing this with IS_NULL(req) makes sense, but would like to
> > > > confirm with you if changing existing behavior is Ok.
> > >
> > > I think it's fine as long as acomp_request_alloc() never returns an
> > > error. Maybe do it in a separate patch first, change IS_ERR_OR_NULL()
> > > to a NULL check in zswap_cpu_comp_dead(), with the reasoning explained
> > > in the changelog, to avoid hiding that change within the bigger patch.
> >
> > Sounds good.
> >
> > >
> > > Actually looking at zswap_cpu_comp_dead(), is the IS_ERR_OR_NULL()
> > > check on acomp_ctx also misleading? Should that also just be a NULL
> > > check?
> >
> > Even a NULL check would be redundant in this case, per my
> > understanding, because if the alloc_percpu() call in
> > zswap_pool_create() had failed, pool creation would have failed.
> >
> > I think a NULL check on the acomp_ctx would still be a good idea, just
> > in case, since this is all part of CPU hotplug. I agree, we don't need
> > an IS_ERR() check on acomp_ctx.
>
> So I think we do one patch to convert both IS_ERR_OR_NULL() to NULL
> checks, and then the current patch 1, right?

Yes.

> > > > Actually looking at zswap_cpu_comp_dead(), is the IS_ERR_OR_NULL()
> > > > check on acomp_ctx also misleading? Should that also just be a NULL
> > > > check?
> > >
> > > Even a NULL check would be redundant in this case, per my
> > > understanding, because if the alloc_percpu() call in
> > > zswap_pool_create() had failed, pool creation would have failed.
> > >
> > > I think a NULL check on the acomp_ctx would still be a good idea, just
> > > in case, since this is all part of CPU hotplug. I agree, we don't need
> > > an IS_ERR() check on acomp_ctx.
> >
> > So I think we do one patch to convert both IS_ERR_OR_NULL() to NULL
> > checks, and then the current patch 1, right?
>
> Yes.

Sounds good, thanks for bearing with me :)

On Mon, Mar 16, 2026 at 03:06:32PM +0000, Yosry Ahmed wrote:
> > > > @@ -786,7 +786,7 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
> > > > return ret;
> > > >
> > > > acomp_ctx->acomp = crypto_alloc_acomp_node(pool->tfm_name, 0, 0, cpu_to_node(cpu));
> > > > - if (IS_ERR(acomp_ctx->acomp)) {
> > > > + if (IS_ERR_OR_NULL(acomp_ctx->acomp)) {
> > > Does crypto_alloc_acomp_node() ever return NULL?
> > > Looking at the error handling just below this check, if this were to
> > > actually return NULL, PTR_ERR(NULL) evaluates to 0. This would cause
> > > the function to incorrectly return 0 (success) instead of an error code,
> > > hiding the allocation failure.
> > 
> > This is a good catch. Just to provide context, this patch was
> > introduced based on Yosry's earlier comments in [1].
> > 
> > [1]: https://patchwork.kernel.org/comment/26282128/
> > 
> > crypto_alloc_acomp_node() currently does not return NULL. However, it
> > could, in future.
> > Since the rest of zswap_cpu_comp_prepare() dereferences
> > acomp_ctx->acomp, it depends on whether we want to future-proof the
> > code to handle a possible eventuality of crypto_alloc_acomp_node()
> > returning NULL.
> 
> Hmm upon revisiting this, I think keeping this as IS_ERR() here is a
> better documentation for the API, and the incossitency between this code
> and acomp_ctx_dealloc() is arguably documenting that the function can
> only return an ERR, but it can also be NULL-initialized by zswap.

Also, sorry for leading you astray in the first place. Looking at this
initially I thought the inconsistency was confusing, but looking at it
with fresh eyes I think it better documents the API and the different
callsites.

On Mon, Mar 16, 2026 at 8:09 AM Yosry Ahmed <yosry@kernel.org> wrote:
>
> On Mon, Mar 16, 2026 at 03:06:32PM +0000, Yosry Ahmed wrote:
> > > > > @@ -786,7 +786,7 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
> > > > > return ret;
> > > > >
> > > > > acomp_ctx->acomp = crypto_alloc_acomp_node(pool->tfm_name, 0, 0, cpu_to_node(cpu));
> > > > > - if (IS_ERR(acomp_ctx->acomp)) {
> > > > > + if (IS_ERR_OR_NULL(acomp_ctx->acomp)) {
> > > > Does crypto_alloc_acomp_node() ever return NULL?
> > > > Looking at the error handling just below this check, if this were to
> > > > actually return NULL, PTR_ERR(NULL) evaluates to 0. This would cause
> > > > the function to incorrectly return 0 (success) instead of an error code,
> > > > hiding the allocation failure.
> > >
> > > This is a good catch. Just to provide context, this patch was
> > > introduced based on Yosry's earlier comments in [1].
> > >
> > > [1]: https://patchwork.kernel.org/comment/26282128/
> > >
> > > crypto_alloc_acomp_node() currently does not return NULL. However, it
> > > could, in future.
> > > Since the rest of zswap_cpu_comp_prepare() dereferences
> > > acomp_ctx->acomp, it depends on whether we want to future-proof the
> > > code to handle a possible eventuality of crypto_alloc_acomp_node()
> > > returning NULL.
> >
> > Hmm upon revisiting this, I think keeping this as IS_ERR() here is a
> > better documentation for the API, and the incossitency between this code
> > and acomp_ctx_dealloc() is arguably documenting that the function can
> > only return an ERR, but it can also be NULL-initialized by zswap.
>
> Also, sorry for leading you astray in the first place. Looking at this
> initially I thought the inconsistency was confusing, but looking at it
> with fresh eyes I think it better documents the API and the different
> callsites.

No worries.