Re: [PATCH v4] ax25: Fix refcount imbalance on inbound connections

[Dan Carpenter <dan.carpenter@linaro.org>]

On Thu, May 23, 2024 at 11:22:43AM -0400, Dan Cross wrote:
> On Thu, May 23, 2024 at 11:05 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > > [snip]
> >
> > I've already said that I don't think the patch is correct and offered
> > an alternative which takes a reference in accept() but also adds a
> > matching put()...  But I can't really test my patch so if we're going to
> > do something that we know is wrong, I'd prefer to just revert Duoming's
> > patch.
> 
> Dan, may I ask how you determined that Lars's patch is incorrect?

The problem is that accept() and ax25_release() are not mirrored pairs.
We're just taking the reference and never dropping it.  Which fixes the
use after free but introduces a leak.

> Testing so far indicates that it works as expected. On the other hand,
> Lars tested your patch and found that it did not address the
> underlying issue
> (https://marc.info/?l=linux-hams&m=171646940902757&w=2).

Yeah.  I've said a couple times that my patch wasn't complete.  I keep
hoping that Duoming will chime in here...

> 
> If I may suggest a path forward, given that observed results show that
> Lars's patch works as expected, perhaps we can commit that and then
> work to incorporate a more robust ref counting strategy a la your
> patch?

The argument for this patch is that it works in testing even though we
think it's not totally correct.  That's not really a good argument.
Like we can revert patches that clearly don't work so we could revert
Duoming's patch, but when we're adding code then that should work.

regards,
dan carpenter