Date: Wed, 13 Oct 2010 14:30:20 -0700 From: Garrett Cooper <gcooper@FreeBSD.org> To: freebsd-hackers@freebsd.org Subject: [PATCH] Bug with powerof2 macro in sys/param.h Message-ID: <AANLkTi=0PSp2KpFTAHTAoosRYdcewiVAxweH4=ivYTLt@mail.gmail.com>
next in thread | raw e-mail | index | archive | help
--0016367b6ecce5ae790492864c55
Content-Type: text/plain; charset=ISO-8859-1
I was talking to someone today about this macro, and he noted that
the algorithm is incorrect -- it fails the base case with ((x) == 0 --
which makes sense because 2^(x) cannot equal 0 (mathematically
impossible, unless you consider the limit as x goes to negative
infinity as log (0) / log(2) is undefined). I tested out his claim and
he was right:
$ ./test_powerof2
Assertion failed: (!powerof2(0)), function main, file test_powerof2.c, line 7.
Abort trap: 6 (core dumped)
Applying the following patch, everything's correct with the
testcase I've written:
$ ./test_powerof2
$
There are a few different places in the sourcebase where this
macro is used, so I'm a bit wary of the implications of having this
patch be committed because this might break functionality somewhere
critical:
sys/dev/cas/if_cas.c: CTASSERT(powerof2(n) && (n) >= (min) && (n) <= (max))
This is ok.
sys/dev/aic7xxx/aic79xx.c: while (powerof2(sg_prefetch_align) == 0)
This is ok.
sys/dev/md/md.c: if (mdio->md_sectorsize != 0 &&
!powerof2(mdio->md_sectorsize))
This is fixed (attached).
sys/dev/mpt/mpt_raid.c:
powerof2(vol_pg->VolumeSettings.HotSparePool)
This is ok.
sys/dev/mpt/mpt_raid.c:
powerof2(disk_pg->PhysDiskSettings.HotSparePool)
This is ok.
sys/dev/gem/if_gem.c:CTASSERT(powerof2(GEM_NRXDESC) && GEM_NRXDESC >=
32 && GEM_NRXDESC <= 8192);
This is ok.
sys/dev/gem/if_gem.c:CTASSERT(powerof2(GEM_NTXDESC) && GEM_NTXDESC >=
32 && GEM_NTXDESC <= 8192);
This is ok.
sys/dev/pci/pci.c: if (!powerof2(actual))
This looks ok.
sys/dev/cxgb/cxgb_sge.c: while (!powerof2(fl_q_size))
sys/dev/cxgb/cxgb_sge.c: while (!powerof2(jumbo_q_size))
I don't feel comfortable enough to state whether or not these are problems.
sys/dev/hme/if_hme.c:CTASSERT(powerof2(HME_NRXDESC) && HME_NRXDESC >=
32 && HME_NRXDESC <= 256);
This is ok.
sys/x86/x86/local_apic.c: KASSERT(powerof2(divisor), ("lapic:
invalid divisor %u", divisor));
This is ok.
sys/x86/x86/local_apic.c: KASSERT(powerof2(count), ("bad count"));
This could be bad if msix is released and this was called, if I was
reading the code correctly.
sys/x86/x86/local_apic.c: KASSERT(powerof2(align), ("bad align"));
This could be bad if msix is released and this was called, if I was
reading the code correctly.
sys/net/flowtable.c: ft->ft_lock_count =
2*(powerof2(mp_maxid + 1) ? (mp_maxid + 1):
This could be bad.
sys/geom/gate/g_gate.c: if (ggio->gctl_sectorsize > 0 &&
!powerof2(ggio->gctl_sectorsize)) {
This is ok.
sys/geom/stripe/g_stripe.c: if (!powerof2(md->md_stripesize)) {
Not sure.
sys/geom/geom_redboot.c: if (powerof2(cp->provider->mediasize))
Not sure.
sys/i386/i386/i686_mem.c: powerof2((len)) && /* ...
and power of two */ \
This is ok.
sys/i386/i386/k6_mem.c: if (desc->mr_len < 131072 ||
!powerof2(desc->mr_len))
This is ok.
sys/i386/pci/pci_pir.c: if (pci_link->pl_irqmask != 0
&& powerof2(pci_link->pl_irqmask))
This is fixed (attached).
sys/i386/pci/pci_pir.c: if (error &&
!powerof2(pci_link->pl_irqmask)) {
Not sure.
sys/netinet6/ip6_input.c: if
(!powerof2(V_ip6_output_flowtable_size)) {
This is ok.
sys/amd64/amd64/amd64_mem.c: powerof2((len)) && /* ...
and power of two */ \
This is ok.
usr.sbin/mptutil/mpt_config.c:#define powerof2(x) ((((x)-1)&(x))==0)
This is fixed (attached).
usr.sbin/mptutil/mpt_config.c: if ((stripe_size < 512)
|| (!powerof2(stripe_size))) {
This is fixed (attached).
usr.sbin/mfiutil/mfi_config.c:#define powerof2(x) ((((x)-1)&(x))==0)
This is fixed (attached).
Could someone please review these patches and help me commit them?
I'm testing out the patches I can (the amd64 and msi and net ones).
Thanks,
-Garrett
--0016367b6ecce5ae790492864c55
Content-Type: text/x-patch; charset=US-ASCII;
name="fix-sys-param-powerof2-macro-when-x-equals-zero.patch"
Content-Disposition: attachment;
filename="fix-sys-param-powerof2-macro-when-x-equals-zero.patch"
Content-Transfer-Encoding: base64
X-Attachment-Id: f_gf8nmnqk0
SW5kZXg6IHN5cy9zeXMvcGFyYW0uaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBzeXMvc3lzL3BhcmFtLmgJKHJl
dmlzaW9uIDIxMTc2NykKKysrIHN5cy9zeXMvcGFyYW0uaAkod29ya2luZyBjb3B5KQpAQCAtMjU0
LDcgKzI1NCw3IEBACiAjZGVmaW5lCXJvdW5kZG93bih4LCB5KQkoKCh4KS8oeSkpKih5KSkKICNk
ZWZpbmUJcm91bmR1cCh4LCB5KQkoKCgoeCkrKCh5KS0xKSkvKHkpKSooeSkpICAvKiB0byBhbnkg
eSAqLwogI2RlZmluZQlyb3VuZHVwMih4LCB5KQkoKCh4KSsoKHkpLTEpKSYofigoeSktMSkpKSAv
KiBpZiB5IGlzIHBvd2VycyBvZiB0d28gKi8KLSNkZWZpbmUgcG93ZXJvZjIoeCkJKCgoKHgpLTEp
Jih4KSk9PTApCisjZGVmaW5lIHBvd2Vyb2YyKHgpCSgoeCkgIT0gMCAmJiAoKCgoeCktMSkmKHgp
KT09MCkpCiAKIC8qIE1hY3JvcyBmb3IgbWluL21heC4gKi8KICNkZWZpbmUJTUlOKGEsYikgKCgo
YSk8KGIpKT8oYSk6KGIpKQo=
--0016367b6ecce5ae790492864c55
Content-Type: application/octet-stream; name="test_powerof2.c"
Content-Disposition: attachment; filename="test_powerof2.c"
Content-Transfer-Encoding: base64
X-Attachment-Id: f_gf8npk301
I2luY2x1ZGUgPHN5cy9wYXJhbS5oPgojaW5jbHVkZSA8YXNzZXJ0Lmg+CgppbnQKbWFpbih2b2lk
KQp7Cglhc3NlcnQgKCFwb3dlcm9mMigwKSk7Cglhc3NlcnQgKHBvd2Vyb2YyKDEpKTsKCWFzc2Vy
dCAocG93ZXJvZjIoMikpOwoJYXNzZXJ0ICghcG93ZXJvZjIoMykpOwoJYXNzZXJ0ICghcG93ZXJv
ZjIoNSkpOwoJYXNzZXJ0IChwb3dlcm9mMig4KSk7CglyZXR1cm4gKDApOwp9Cg==
--0016367b6ecce5ae790492864c55
Content-Type: text/x-patch; charset=US-ASCII;
name="mfiutil+mptutil-use-sys-param-powerof2.patch"
Content-Disposition: attachment;
filename="mfiutil+mptutil-use-sys-param-powerof2.patch"
Content-Transfer-Encoding: base64
X-Attachment-Id: f_gf8nx1wt2
SW5kZXg6IHVzci5zYmluL21maXV0aWwvbWZpX2NvbmZpZy5jCj09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIHVzci5z
YmluL21maXV0aWwvbWZpX2NvbmZpZy5jCShyZXZpc2lvbiAyMTE3NjcpCisrKyB1c3Iuc2Jpbi9t
Zml1dGlsL21maV9jb25maWcuYwkod29ya2luZyBjb3B5KQpAQCAtMjksNiArMjksNyBAQAogICog
JEZyZWVCU0QkCiAgKi8KIAorI2luY2x1ZGUgPHN5cy9wYXJhbS5oPgogI2luY2x1ZGUgPHN5cy90
eXBlcy5oPgogI2lmZGVmIERFQlVHCiAjaW5jbHVkZSA8c3lzL3N5c2N0bC5oPgpAQCAtNTIsOCAr
NTMsNiBAQAogc3RhdGljIGludAlhZGRfc3BhcmUoaW50IGFjLCBjaGFyICoqYXYpOwogc3RhdGlj
IGludAlyZW1vdmVfc3BhcmUoaW50IGFjLCBjaGFyICoqYXYpOwogCi0jZGVmaW5lIHBvd2Vyb2Yy
KHgpICAgICgoKCh4KS0xKSYoeCkpPT0wKQotCiBzdGF0aWMgbG9uZwogZGVodW1hbml6ZShjb25z
dCBjaGFyICp2YWx1ZSkKIHsKSW5kZXg6IHVzci5zYmluL21wdHV0aWwvbXB0X2NvbmZpZy5jCj09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT0KLS0tIHVzci5zYmluL21wdHV0aWwvbXB0X2NvbmZpZy5jCShyZXZpc2lvbiAyMTE3
NjcpCisrKyB1c3Iuc2Jpbi9tcHR1dGlsL21wdF9jb25maWcuYwkod29ya2luZyBjb3B5KQpAQCAt
NTAsOCArNTAsNiBAQAogc3RhdGljIHZvaWQJZHVtcF9jb25maWcoQ09ORklHX1BBR0VfUkFJRF9W
T0xfMCAqdm9sKTsKICNlbmRpZgogCi0jZGVmaW5lIHBvd2Vyb2YyKHgpICAgICgoKCh4KS0xKSYo
eCkpPT0wKQotCiBzdGF0aWMgbG9uZwogZGVodW1hbml6ZShjb25zdCBjaGFyICp2YWx1ZSkKIHsK
--0016367b6ecce5ae790492864c55
Content-Type: text/x-patch; charset=US-ASCII;
name="sys-md-md-fix-powerof2-invocation.patch"
Content-Disposition: attachment;
filename="sys-md-md-fix-powerof2-invocation.patch"
Content-Transfer-Encoding: base64
X-Attachment-Id: f_gf8o0v5h3
SW5kZXg6IHN5cy9kZXYvbWQvbWQuYwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBzeXMvZGV2L21kL21kLmMJKHJl
dmlzaW9uIDIxMTc2NykKKysrIHN5cy9kZXYvbWQvbWQuYwkod29ya2luZyBjb3B5KQpAQCAtODMy
LDcgKzgzMiw3IEBACiAJZXJyb3IgPSAwOwogCWlmIChtZGlvLT5tZF9vcHRpb25zICYgfihNRF9B
VVRPVU5JVCB8IE1EX0NPTVBSRVNTIHwgTURfUkVTRVJWRSkpCiAJCXJldHVybiAoRUlOVkFMKTsK
LQlpZiAobWRpby0+bWRfc2VjdG9yc2l6ZSAhPSAwICYmICFwb3dlcm9mMihtZGlvLT5tZF9zZWN0
b3JzaXplKSkKKwlpZiAoIXBvd2Vyb2YyKG1kaW8tPm1kX3NlY3RvcnNpemUpKQogCQlyZXR1cm4g
KEVJTlZBTCk7CiAJLyogQ29tcHJlc3Npb24gZG9lc24ndCBtYWtlIHNlbnNlIGlmIHdlIGhhdmUg
cmVzZXJ2ZWQgc3BhY2UgKi8KIAlpZiAobWRpby0+bWRfb3B0aW9ucyAmIE1EX1JFU0VSVkUpCg==
--0016367b6ecce5ae790492864c55
Content-Type: text/x-patch; charset=US-ASCII;
name="sys-i386-pci-pci_pir-remove-unnecessary-powerof2-clause.patch"
Content-Disposition: attachment;
filename="sys-i386-pci-pci_pir-remove-unnecessary-powerof2-clause.patch"
Content-Transfer-Encoding: base64
X-Attachment-Id: f_gf8pce9p4
SW5kZXg6IHN5cy9pMzg2L3BjaS9wY2lfcGlyLmMKPT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gc3lzL2kzODYvcGNp
L3BjaV9waXIuYwkocmV2aXNpb24gMjEzODAyKQorKysgc3lzL2kzODYvcGNpL3BjaV9waXIuYwko
d29ya2luZyBjb3B5KQpAQCAtNTI2LDcgKzUyNiw3IEBACiAJICogSVJRcyIgc2V0LgogCSAqLwog
CWlmICghUENJX0lOVEVSUlVQVF9WQUxJRChwY2lfbGluay0+cGxfaXJxKSkgewotCQlpZiAocGNp
X2xpbmstPnBsX2lycW1hc2sgIT0gMCAmJiBwb3dlcm9mMihwY2lfbGluay0+cGxfaXJxbWFzaykp
CisJCWlmIChwb3dlcm9mMihwY2lfbGluay0+cGxfaXJxbWFzaykpCiAJCQlpcnEgPSBmZnMocGNp
X2xpbmstPnBsX2lycW1hc2spIC0gMTsKIAkJZWxzZQogCQkJaXJxID0gcGNpX3Bpcl9jaG9vc2Vf
aXJxKHBjaV9saW5rLAo=
--0016367b6ecce5ae790492864c55--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTi=0PSp2KpFTAHTAoosRYdcewiVAxweH4=ivYTLt>