On Thu, Nov 14, 2024 at 12:52 PM Christian Brauner <brauner@xxxxxxxxxx> wrote: > > On Thu, Nov 14, 2024 at 11:05:36AM +0100, Amir Goldstein wrote: > > ovl_setup_cred_for_create() decrements one refcount of new creds and > > ovl_revert_creds() in callers decrements the last refcount. > > > > In preparation to revert_creds_light() back to caller creds, pass an > > explicit reference of the creators creds to the callers and drop the > > refcount explicitly in the callers after ovl_revert_creds(). > > > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx> > > --- > > > > Miklos, Christian, > > > > I was chasing a suspect memleak in revert_creds_light() patches. > > This fix is unrelated to memleak but I think it is needed for > > correctness anyway. > > > > This applies in the middle of the series after adding the > > ovl_revert_creds() helper. > > Ok, so you're moving the put_cred() on the cred_for_create creds into > the callers. Tbh, that patch alone here is very confusing because with > the last patch in your tree at 07532f7f8450 you're calling > > old_cred = override_creds(override_cred); > > which made it buggy. But I see in 3756f22061c2 this is fixed and > correctly uses > > old_cred = override_creds_light(override_cred); > > as expected. And together with that change your patch here makes perfect > sense. I don't want to complain too much but it would've been nice if > that was spelled out in the commit message. Would've spared me 30 > minutes of staring at the code until I refreshed your branch. :) No, do feel free to complain :) At the time of writing the commit message I still did not understand the bug. It just made sense to me that the reference should be passed explicitly, because I *thought* that ovl_setup_cred_for_create() was dropping the last reference after the last commit, but that wasn't going to explain a memleak (quite the opposite). My point with *this* patch is that it was more clear to code readers who is responsible to drop the reference and after a while, this clarity also helped me to see the bug. > > Reviewed-by: Christian Brauner <brauner@xxxxxxxxxx> Thanks! Amir.
