Re: [PATCH] virt-aa-helper: delete dynamic files

On Tue, Apr 01, 2025 at 03:24:10PM +0100, Daniel P. Berrangé wrote: > On Tue, Apr 01, 2025 at 07:15:46AM -0700, Andrea Bolognani via Devel wrote: > > More importantly, I'm not convinced that you can just start deleting > > that file unconditionally. Since, as you note, it's explicitly > > documented that the user might add local customizations to the > > profile, deleting it might result in the loss of those local changes. > > > > So I think you need something more like:> > > > expected = generate_default_profile(); > > actual = read_existing_profile(); > > if (STREQ(actual, expected)) { > > delete_profile(); > > } > > > > In other words, only delete the existing profile if you can prove > > that it contains no valuable information. > > Not sure its that's simple as we've changed what's in the profile > over time, especially if the host in question upgraded from > apparmor 2.x to 3.x, so such a comparison is likely to get false > results a decent amount of the time. You're right about the AppArmor 2/3 part. And of course the admin might have made local modifications to the template over time too, which a simplistic approach like the one I've described above would not capture correctly. Still, false positives (i.e. the profiles incorrectly being detected as containing local modifications) are not that harmful, as they only result in lack of cleanup, which is the previous status quo. So perhaps that's tolerable. > It might need to be more of a config file setting to allow people > turn off, and/or a build time default if a distro really wants > to preserve old behaviour a bit longer I'm not a big fan of having to introduce yet another tunable. If it really must exist, I think it should be a "force cleanup ignoring local modification detection logic" bool.