On Thu, Mar 18, 2021 at 10:48 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > On Wed, Mar 3, 2021 at 3:54 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote: > > The situation has changed a bit since that was written, though... Back > > then after the policy had been loaded there was no way to turn back > > and if sel_make_policy_nodes() failed, the new policy would stay and > > selinuxfs would have been left behind in an inconsistent/broken state. > > Now this issue is fixed and the new policy isn't actually applied > > until the selinuxfs preparation succeeds. So from a certain POV, the > > selinuxfs failure is no longer that fatal and could just print a > > warning like the other error path, because the result is the same > > after both failures (active policy and selinuxfs state remains > > unchanged). > > > > Paul (or Stephen if you are reading this and have time to comment), > > what do you think? > > Sorry for the late reply, I lost this in my inbox and since I already > marked the patchset as "changes requested" in patchwork it fell off my > radar ... > > Anyway, back to your question ... it does seem like pr_warn() is the > right answer here for the reasons that Ondrej mentioned above, and I > personally feel it is in keeping with the original patch's intention > as well; "If the policy fails to be loaded from userspace then a > warning message is printed ..." However, I'm not going to lose a lot > of sleep over differences between pr_warn() and pr_err() here, if > someone feels strongly that it should be pr_err() and can back that up > with some solid reasoning and/or precedence then so be it. That's fine with me. FWIW, I think the rationale for using pr_warn_ratelimited() for error returns from security_load_policy() was that the failure could be entirely userspace-induced, i.e. just pass the kernel an invalid policy. The pr_err() messages on sel_make_bools/classes were in contrast entirely kernel-internal errors and could leave the system in an inconsistent state as you noted.
