[PATCH] Check if the idmap_hash range is big enough
Michael Adam
obnox at samba.org
Fri Feb 17 17:44:34 UTC 2017
On 2017-02-17 at 12:24 +0100, Andreas Schneider wrote:
> On Wednesday, 15 February 2017 23:08:19 CET Michael Adam wrote:
> > Regarding your patch:
> >
> > - The check for 500000 seems a bit heuristic to me.
> > - I (still) don't like the fact that the testparm binary
> > tests stuff for the modules which are kind of
> > separate.
>
> We need to tell the user somehow what he does wrong. Why ist testparm not the
> tool to do that?
Because the one and only code place with knowledge about
an idmap module's config should be ... the idmap module.
Just like with VFS modules.
If we check the config and output warnings or errors in
testparm, then the CODE for the check and the warning should
ideally be in the idmap (or vfs ...) module.
This way the same code could be called at startup and lead
to an abort or a warning, depending on the error. Couldn't
the check be called in lp_load? I don't fully understand
anyways, why testparm does more checks after calling
lp_load ... ;-)
Hence I am saying the testparm.c code is conceptually the
wrong place for such a check. I think I said something
like that already in the last wave of config check improvements.
If the idmap module changes, testparm needs to be changed as
well.
Of course it is much more easy to put the check into testparm.
But not good. :-)
So, these are my concerns.
Cheers - Michael
> New improved patchset attached.
>
>
> Andreas
>
>
> --
> Andreas Schneider GPG-ID: CC014E3D
> Samba Team asn at samba.org
> www.samba.org
> From 675d755dd85324d156b038f5ac5250470b9d350e Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Wed, 15 Feb 2017 08:55:24 +0100
> Subject: [PATCH 1/2] docs: Improve the idmap_hash manpage
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=12582
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> docs-xml/manpages/idmap_hash.8.xml | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/docs-xml/manpages/idmap_hash.8.xml b/docs-xml/manpages/idmap_hash.8.xml
> index 9f4f1d1933c..8e9fa82960d 100644
> --- a/docs-xml/manpages/idmap_hash.8.xml
> +++ b/docs-xml/manpages/idmap_hash.8.xml
> @@ -24,6 +24,11 @@
> to support a local name mapping files if enabled via the
> "winbind normalize names" and "winbind nss info"
> parameters in smb.conf.
> + The module divides the the range into subranges for each domain that is being
> + handled by the idmap config.
> + Each range has a size of roughly 525000 IDs (20 bit). This means
> + that the range for multiple domains needs to be large enough! So a good value
> + is normally '100000-4000000' or even bigger.
You list 4 million here, i.e. space for 8 domains. Note that
unlike the autorid module, the hash module does not take the
next free range for the next domain that comes across, but for
each sid, the range is determined by the 12bit (!!!) hash that
is calculated from the sid. I.e. each sid has its pre-defined
range. I.e. you need all the 4096 (12bit) ranges if you want
to cover all possible domains that might come across.
Also, there is no addition of the low-id of the range to a rid
or so, so if you want to catch all possible SIDs that come
across your "idmap config *" setup you need a range of
0 - 2147483648 = 524288 * 4096
if my math is not entirely wrong.
(Btw, the 524288 elements are 19 bit... (= 1<<19),
whatever I had written before. ;-)
If you have a smaller range, some sids won't get mapped.
The example below lists 4 billon as upper limit.
That is enough.
> </para>
> </refsynopsisdiv>
>
> @@ -53,7 +58,7 @@
> <programlisting>
> [global]
> idmap config * : backend = hash
> - idmap config * : range = 1000-4000000000
> + idmap config * : range = 100000-4000000000
If you want to catch as much as possible of a domain
that gets hashed to 0, then the lower bound needs to
be as low as possible, hence the 1000. But then,
the first 1000 rids in a domain will be used as well,
and hence why not skip this first range entirely
and start at 500000 ? ;-)
So:
- the idmap hash module, when used for "idmap config *",
ideally should have the full range of
0 - 2147483648 which is not quite possible (at the low
end at least)...
- domains occupy a fixed range. there is nothing that
can change that.
- the domains are mapped to a 12 bit hash which is very
likely to show collisions.
- each domain has a fixed range size and if there are
larger rids, they are not filtered but
rid and rid % 524288 get mapped to the same ID
==> If we could only delete this module. :-/
It is so full of serious and dangerous design flaws.
But unfortunately it is used out there.
Not quite sure where to go from here,
but this is my analysis.
Cheers - Michael
> winbind nss info = hash
> winbind normalize names = yes
> --
> 2.11.0
>
>
> From f7659c49c43fe7a25f3e27f02585ac0c8ad878b3 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Wed, 15 Feb 2017 10:12:51 +0100
> Subject: [PATCH 2/2] s3-testparm: Add an error for small idmap_hash ranges
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=12582
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> source3/utils/testparm.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/source3/utils/testparm.c b/source3/utils/testparm.c
> index 3e80c39cf9d..9010c4e4d85 100644
> --- a/source3/utils/testparm.c
> +++ b/source3/utils/testparm.c
> @@ -190,6 +190,22 @@ static bool do_idmap_check(void)
> goto done;
> }
> }
> +
> + ok = strequal(c->backend, "hash");
> + if (ok) {
> + if (c->high - c->low < 524288) {
> + fprintf(stderr,
> + "ERROR: The idmap range for '%s' "
> + "configrued for the the domain "
> + "'%s' is too small! Please consult "
> + "the manpage of idmap_hash module."
> + "\n\n",
> + c->backend,
> + c->domain_name);
> + ok = false;
> + goto done;
> + }
> + }
> }
>
> ok = true;
> --
> 2.11.0
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 163 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20170217/dc774bec/signature.sig>
More information about the samba-technical
mailing list