On Wed, Apr 24, 2024 at 10:01 PM Vinicius Costa Gomes <vinicius.gomes@xxxxxxxxx> wrote: > > Miklos Szeredi <miklos@xxxxxxxxxx> writes: > > > On Wed, 3 Apr 2024 at 04:18, Vinicius Costa Gomes > > <vinicius.gomes@xxxxxxxxx> wrote: > > > >> - 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. > > > > Don't know. Well, there's nesting (in ovl_nlink_end()) but I don't > > see why that should be an issue. > > > > I see why Amir suggested moving away from scoped guards, but that also > > introduces the possibility of subtle bugs if we don't audit every one > > of those sites carefully... > > > > Maybe patchset should be restructured to first do the > > override_creds_light() conversion without guards, and then move over > > to guards. Or the other way round, I don't have a preference. But > > mixing these two independent changes doesn't sound like a great idea > > in any case. > > Sounds good. Here's I am thinking: > > patch 1: introduce *_creds_light() > patch 2: move backing-file.c to *_creds_light() > patch 3: move overlayfs to *_creds_light() > patch 4: introduce the guard helpers > patch 5: move backing-file.c to the guard helpers > patch 6: move overlayfs to the guard helpers > > (and yeah, the subject of the patches will be better than these ;-) > > Is this what you had in mind? > I think this series would make a lot of sense. It first addresses the issue that motivated your work and I expect patch 3 would be rather simple to review. Please use your best judgement to break patch 6 into more chewable pieces because the current ovl patch is quite large to review in one go. I will leave it up to you to find the right balance. Also w.r.t the guard() vs. scoped_guard() question, remember that there is another option that may be better than either in some cases - re-factoring to a helper with a guard(). One example that jumps to me is ovl_copyfile() - seems nicer to add a guard() in all the specific helpers then adding the scoped_guard() around the switch statement. But really this is a question of taste and avoiding unneeded code churn and unneeded nested scopes. There is no one clear way, so please use your judgement per case and we can debate on your choices in review. Thanks, Amir.
