On 9/27/23, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > On Wed, 27 Sept 2023 at 07:10, Christian Brauner <brauner@xxxxxxxxxx> > wrote: >> >> No need to resend I can massage this well enough in-tree. > > Hmm. Please do, but here's some more food for thought for at least the > commit message. > > Because there's more than the "__fput_sync()" issue at hand, we have > another delayed thing that this patch ends up short-circuiting, which > wasn't obvious from the original description. > > I'm talking about the fact that our existing "file_free()" ends up > doing the actual release with > > call_rcu(&f->f_rcuhead, file_free_rcu); > > and the patch under discussion avoids that part too. > Comments in the patch explicitly mention dodgin RCU for the file object. > And I actually like that it avoids it, I just think it should be > mentioned explicitly, because it wasn't obvious to me until I actually > looked at the old __fput() path. Particularly since it means that the > f_creds are free'd synchronously now. > Well put_cred is called synchronously, but should this happen to be the last ref on them, they will get call_rcu(&cred->rcu, put_cred_rcu)'ed. > I do think that's fine, although I forget what path it was that > required that rcu-delayed cred freeing. Worth mentioning, and maybe > worth thinking about. > See above. The only spot which which plays tricks with it is faccessat, other than that all creds are explicitly freed with rcu. > However, when I *did* look at it, it strikes me that we could do this > differently. > > Something like this (ENTIRELY UNTESTED) patch, which just moves this > logic into fput() itself. > I did not want to do it because failed open is a special case, quite specific to one syscall (and maybe few others later). As is you are adding a branch to all final fputs and are preventing whacking that 1 -> 0 unref down the road, unless it gets moved out again like in my patch. -- Mateusz Guzik <mjguzik gmail.com>
