On Wed, Apr 24, 2024 at 12:15:25PM -0700, Vinicius Costa Gomes wrote: > Christian Brauner <brauner@xxxxxxxxxx> writes: > > > On Tue, Apr 02, 2024 at 07:18:05PM -0700, Vinicius Costa Gomes wrote: > >> Hi, > >> > >> Changes from RFC v3: > >> - Removed the warning "fixes" patches, as they could hide potencial > >> bugs (Christian Brauner); > >> - Added "cred-specific" macros (Christian Brauner), from my side, > >> added a few '_' to the guards to signify that the newly introduced > >> helper macros are preferred. > >> - Changed a few guard() to scoped_guard() to fix the clang (17.0.6) > >> compilation error about 'goto' bypassing variable initialization; > >> > >> Link to RFC v3: > >> > >> https://lore.kernel.org/r/20240216051640.197378-1-vinicius.gomes@xxxxxxxxx/ > >> > >> Changes from RFC v2: > >> - Added separate patches for the warnings for the discarded const > >> when using the cleanup macros: one for DEFINE_GUARD() and one for > >> DEFINE_LOCK_GUARD_1() (I am uncertain if it's better to squash them > >> together); > >> - Reordered the series so the backing file patch is the first user of > >> the introduced helpers (Amir Goldstein); > >> - Change the definition of the cleanup "class" from a GUARD to a > >> LOCK_GUARD_1, which defines an implicit container, that allows us > >> to remove some variable declarations to store the overriden > >> credentials (Amir Goldstein); > >> - Replaced most of the uses of scoped_guard() with guard(), to reduce > >> the code churn, the remaining ones I wasn't sure if I was changing > >> the behavior: either they were nested (overrides "inside" > >> overrides) or something calls current_cred() (Amir Goldstein). > >> > >> New questions: > >> - The backing file callbacks are now called with the "light" > >> overriden credentials, so they are kind of restricted in what they > >> can do with their credentials, is this acceptable in general? > > > > Until we grow additional users, I think yes. Just needs to be > > documented. > > > > Will add some documentation for it, then. > > >> - in ovl_rename() I had to manually call the "light" the overrides, > >> both using the guard() macro or using the non-light version causes > >> the workload to crash the kernel. I still have to investigate why > >> this is happening. Hints are appreciated. > > > > Do you have a reproducer? Do you have a splat from dmesg? > > Just to be sure, with this version of the series the crash doesn't > happen. It was only happening when I was using the guard() macro > everywhere. > > I just looked at my crash collection and couldn't find the splats, from > what I remember I lost connection to the machine, and wasn't able to > retrieve the splat. > > I believe the crash and clang 17 compilation error point to the same > problem, that in ovl_rename() some 'goto' skips the declaration of the > (implicit) variable that the guard() macro generates. And it ends up > doing a revert_creds_light() on garbage memory when ovl_rename() > returns. If this is a compiler bug this warrants at least a comment in the commit message because right now people will be wondering why that place doesn't use a guard. Ideally we can just use guards everywhere though and report this as a bug against clang, I think. > > (if you want I can try and go back to "guard() everywhere" and try a bit > harder to get a splat) > > Does that make sense? Yes.
