On Wed, Nov 22, 2023 at 12:35 PM Jacob Satterfield <jsatterfield.linux@xxxxxxxxx> wrote: > On Mon, Nov 20, 2023 at 8:29 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > > > I'm a concerned about all the work we have to do just to count the > > conditional rules. Other than not working with existing binary > > policies, have you looked at bumping the policy version and introducing > > a binary format change that would include the number of conditional > > rules? > > Thanks for raising the issue. I had considered adding the total size > of the conditional table to the binary policy, but I wasn't sure if it > would be substantive enough to warrant bumping the policy version. I appreciate the thoughtfulness, but the version field in the policy is 32-bits and we are currently up to policy format v33 so we've still got a few version numbers to play with :) > As > you point out, the counting work will be needed for existing binary > policies making this patch necessary for the default case, but if you > are concerned about the performance penalty this patch brings (which > is less than the gain provided by the avtab array patch), then there > are two threads to possibly be worked on. To be clear, I'm not thinking about supporting this for existing policies, just the new policy format; the existing policy versions would behave as they do now ... although if we do the array conversion we will likely need to do some type of realloc()/retry or something, I dunno, I'll leave that to you to brainstorm ;) > One is to rework this patch to include more invasive changes to count > rules without actually reading and destroying nodes thus saving cycles > but requiring more lines of code. Because policy parsing is not > handled separately from the construction of the policydb structure > (they are deeply intertwined), I was reluctant to add more complexity > just to have a parse-only code path. Would you prefer speed or simpler > logic for older policies? That's the problem we have right now. We have to do a lot of work (allocations, etc.) that we throw away in the case where we are counting, not to mention that bolting on the count-only functionality is kinda hacky/ugly (not your fault, that is just the way the code is right now). As you mention, the alternative is to significantly rework how we parse/load the policy, and that isn't a very exciting prospect as far as I'm concerned. I'm not sure if moving over to flex array is a win, I suspect that whatever we gain in memory savings we lose in not having direct access. I dunno, maybe it wouldn't be too bad. I'm open to ideas here ... I'm looking for something that would support the improvements for a new policy with an explicit count, while still falling back to something that works "reasonably well" in the current case where we have to guess. In this case "reasonably well" means no worse than current in terms of performance and memory use while not over complicating the code. -- paul-moore.com
