From: Paul Moore <paul@paul-moore.com>
To: "Christian Göttsche" <cgzones@googlemail.com>, selinux@vger.kernel.org
Cc: Stephen Smalley <stephen.smalley.work@gmail.com>,
Eric Paris <eparis@parisplace.org>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 2/7] selinux: use u32 as bit type in ebitmap code
Date: Wed, 09 Aug 2023 19:07:41 -0400 [thread overview]
Message-ID: <67cee6245e2895e81a0177c4c1ed01ba.paul@paul-moore.com> (raw)
In-Reply-To: <20230807171143.208481-1-cgzones@googlemail.com>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 3221 bytes --]
On Aug 7, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@googlemail.com> wrote:
>
> The extensible bitmap supports bit positions up to U32_MAX due to the
> type of the member highbit being u32. Use u32 consistently as the type
> for bit positions to announce to callers what range of values is
> supported.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
> v3:
> - revert type change of unrelated iter variable
> - use U32_MAX instead of (u32)-1
> v2: avoid declarations in init-clauses of for loops
> ---
> security/selinux/ss/ebitmap.c | 29 +++++++++++++++--------------
> security/selinux/ss/ebitmap.h | 32 ++++++++++++++++----------------
> 2 files changed, 31 insertions(+), 30 deletions(-)
...
> diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
> index 77875ad355f7..a313e633aa8e 100644
> --- a/security/selinux/ss/ebitmap.c
> +++ b/security/selinux/ss/ebitmap.c
> @@ -471,18 +472,18 @@ int ebitmap_read(struct ebitmap *e, void *fp)
> int ebitmap_write(const struct ebitmap *e, void *fp)
> {
> struct ebitmap_node *n;
> - u32 count;
> + u32 bit, count, last_bit, last_startbit;
> __le32 buf[3];
> u64 map;
> - int bit, last_bit, last_startbit, rc;
> + int rc;
>
> buf[0] = cpu_to_le32(BITS_PER_U64);
>
> count = 0;
> last_bit = 0;
> - last_startbit = -1;
> + last_startbit = U32_MAX;
> ebitmap_for_each_positive_bit(e, n, bit) {
> - if (rounddown(bit, (int)BITS_PER_U64) > last_startbit) {
> + if (last_startbit == U32_MAX || rounddown(bit, BITS_PER_U64) > last_startbit) {
I'm getting worried about what might happen if the ebitmap starts to
contain bits near the end of the range, e.g. U32_MAX. When lastbit
was signed this was a non-issue as we could set it to a negative
value (-1) and not worry about it, although the maximum value
difference between the signed and unsigned types would eventually be
a problem.
While looking closer at this loop, I'm now wondering if we shouldn't
just rewrite the logic a bit to simplify things, and possibly speed
it up a small amount. How about something like this:
count = 1;
n = e->node;
while (n->next) {
count++;
n = n->next;
}
last_startbit = n->startbit;
last_bit = n->startbit + find_last_bit(n->maps, EBITMAP_SIZE);
You should probably verify that there isn't something stupid like an
off-by-one bug in the code above, but I think it is a lot cleaner
than what we currently have and should resolve a lot of the type/math
issues.
> count++;
> last_startbit = rounddown(bit, BITS_PER_U64);
> }
> @@ -496,9 +497,9 @@ int ebitmap_write(const struct ebitmap *e, void *fp)
> return rc;
>
> map = 0;
> - last_startbit = INT_MIN;
> + last_startbit = U32_MAX;
> ebitmap_for_each_positive_bit(e, n, bit) {
> - if (rounddown(bit, (int)BITS_PER_U64) > last_startbit) {
> + if (last_startbit == U32_MAX || rounddown(bit, BITS_PER_U64) > last_startbit) {
> __le64 buf64[1];
Similar to the above, I think we can probably rewrite this to simply
walk the ebitmap nodes and write them out. Using
ebitmap_for_each_positive_bit() seems overly complicated to me,
although I may be missing something important and obvious ...
--
paul-moore.com
next prev parent reply other threads:[~2023-08-09 23:07 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-07 17:11 [PATCH v3 2/7] selinux: use u32 as bit type in ebitmap code Christian Göttsche
2023-08-07 17:11 ` [PATCH v3 3/7] selinux: update type for number of class permissions in services code Christian Göttsche
2023-08-09 23:07 ` Paul Moore
2023-08-07 17:11 ` [PATCH v3 4/7] selinux: make left shifts well defined Christian Göttsche
2023-08-09 23:07 ` Paul Moore
2023-08-07 17:11 ` [PATCH v3 5/7] selinux: avoid implicit conversions in selinuxfs code Christian Göttsche
2023-08-09 23:07 ` Paul Moore
2023-08-07 17:11 ` [PATCH v3 6/7] selinux: avoid implicit conversions in policydb code Christian Göttsche
2023-08-09 23:07 ` Paul Moore
2023-08-07 17:11 ` [PATCH v3 7/7] selinux: use unsigned iterator in nlmsgtab code Christian Göttsche
2023-08-09 23:07 ` Paul Moore
2023-08-07 17:11 ` [PATCH v3 1/7] selinux: avoid implicit conversions in avtab code Christian Göttsche
2023-08-09 23:07 ` Paul Moore
2023-08-09 23:07 ` Paul Moore [this message]
2023-08-16 15:00 ` [PATCH v3 2/7] selinux: use u32 as bit type in ebitmap code Christian Göttsche
2023-08-18 13:55 ` Christian Göttsche
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=67cee6245e2895e81a0177c4c1ed01ba.paul@paul-moore.com \
--to=paul@paul-moore.com \
--cc=cgzones@googlemail.com \
--cc=eparis@parisplace.org \
--cc=linux-kernel@vger.kernel.org \
--cc=selinux@vger.kernel.org \
--cc=stephen.smalley.work@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).