On Wed, 2025-07-02 at 10:20 -0700, Jeff Johnson wrote: > > Internally Sarika had a 2-patch series where the first patch attempts to make > sure cfg80211_sinfo_release_content() is only called when needed. However that > required either separating out the error cases into separate gotos or calling > cfg80211_sinfo_release_content() at each error point, and I felt that > unnecessarily made the code less readable & maintainable, especially given > that the 2nd patch (which is the one she posted) addresses the underlying > problem (and is similar to the approach Dan posted where he zeroes sinfo at > the end of cfg80211_sinfo_release_content()). OK but then the second one isn't really necessary at all? > In my mind this is no different than the numerous places where we blindly call > kfree() with a pointer that may or may not be NULL. It seems a bit different to me - we don't usually write code like foo = ...; kfree(foo); foo = NULL; ... kfree(foo); return xyz; and here we quite explicitly get what looks like double free. And is, right now, but I don't really think I'd want the code to look like it? > But if you want the other patch as well, we can continue our internal review > of it. I think I'd probably prefer only that one? But I guess one could make an argument that it's safer to NULL it out so it cannot be used at all, neither to double-free nor to access in any other way. The one Dan just posted as a sibling to your reply also contains a _missing_ call, i.e. a fix for a leak. But we wanted to do some additional work in this area anyway. Maybe the right thing to do would be to have some kind of context/guard instead? johannes
