Bug 2031015
| Summary: | zlib: Unnecessary IFUNC resolver for crc32_z | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Red Hat Enterprise Linux 8 | Reporter: | Florian Weimer <fweimer> | ||||||||||
| Component: | zlib | Assignee: | Lukas Javorsky <ljavorsk> | ||||||||||
| Status: | CLOSED ERRATA | QA Contact: | Dita Stehlikova <dstehlik> | ||||||||||
| Severity: | low | Docs Contact: | |||||||||||
| Priority: | unspecified | ||||||||||||
| Version: | 8.5 | CC: | bugproxy, databases-maint, dstehlik, hhorak, iii, ljavorsk, pkubat, tuliom, zmiklank | ||||||||||
| Target Milestone: | rc | Keywords: | Triaged | ||||||||||
| Target Release: | --- | Flags: | pm-rhel:
mirror+
|
||||||||||
| Hardware: | x86_64 | ||||||||||||
| OS: | Linux | ||||||||||||
| Whiteboard: | |||||||||||||
| Fixed In Version: | zlib-1.2.11-25.el8 | Doc Type: | No Doc Update | ||||||||||
| Doc Text: | Story Points: | --- | |||||||||||
| Clone Of: | |||||||||||||
| : | 2031019 (view as bug list) | Environment: | |||||||||||
| Last Closed: | 2023-11-14 15:49:21 UTC | Type: | Bug | ||||||||||
| Regression: | --- | Mount Type: | --- | ||||||||||
| Documentation: | --- | CRM: | |||||||||||
| Verified Versions: | Category: | --- | |||||||||||
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||||||
| Cloudforms Team: | --- | Target Upstream Version: | |||||||||||
| Embargoed: | |||||||||||||
| Bug Depends On: | |||||||||||||
| Bug Blocks: | 2031019 | ||||||||||||
| Attachments: |
|
||||||||||||
Just to make it clear, can you please confirm you're talking about optimization that was requested from us in this BUG: bug#1959423? (In reply to Lukas Javorsky from comment #2) > Just to make it clear, can you please confirm you're talking about > optimization that was requested from us in this BUG: bug#1959423? That is a bug for Red Hat Enterprise Linux 9. I was mistaken, and the IFUNC resolver was actually introduced for POWER8 support, as part of bug 1666798. > I was mistaken, and the IFUNC resolver was actually introduced for POWER8 support, as part of bug 1666798.
Indeed. Would you like to mirror this issue to IBM?
(In reply to Tulio Magno Quites Machado Filho from comment #5) > > I was mistaken, and the IFUNC resolver was actually introduced for POWER8 support, as part of bug 1666798. > > Indeed. Would you like to mirror this issue to IBM? Sure, please go ahead. Thanks. ------- Comment From mscastanho 2021-12-16 13:02 EDT------- As mentioned before, this was added by a patch to provide an optimized version of crc32 for POWER8. There is actually a revised version of that patch waiting for review upstream [1][2]. That one explicitly checks the architecture to make sure it only adds the resolver for powerpc, so should avoid the problem on x86. Could we use that patch instead? [1] https://github.com/madler/zlib/pull/478 [2] https://patch-diff.githubusercontent.com/raw/madler/zlib/pull/478.patch As already consulted before in case of other similar optimization bugs for zlib, we likely cannot expect the upstream to accept this as zlib upstream is not that active regarding these changes. Since we have already optimization patches in zlib maintained by IBM, we can likely add this to RHEL (proper re-check whether the patch applies cleanly should likely be still done, but I expect it is prepared carefully). For the verification -- can we again count on IBM verifying it once we have a testing RPM build? Anyway, our capacity is already exhausted for RHEL-8.6.0 release and we would like to apply changes in such basic libraries that other components depend on early in the development process (which we're way beyond now). Therefore I'd like to propose this to RHEL-8.7.0. Please, provide more justification if the decision of not fixing this for 8.6.0 has a too negative impact. ------- Comment From tulioqm.com 2022-01-25 10:16 EDT------- > For the verification -- can we again count on IBM verifying it once we have > a testing RPM build? Yes. We can test this. > Anyway, our capacity is already exhausted for RHEL-8.6.0 release and we > would like to apply changes in such basic libraries that other components > depend on early in the development process (which we're way beyond now). > Therefore I'd like to propose this to RHEL-8.7.0. Please, provide more > justification if the decision of not fixing this for 8.6.0 has a too > negative impact. It's unclear to me if this question was targeting IBM. Anyway, from my point of view, it's OK to delay it for 8.7. (In reply to Honza Horak from comment #8) > Anyway, our capacity is already exhausted for RHEL-8.6.0 release and we > would like to apply changes in such basic libraries that other components > depend on early in the development process (which we're way beyond now). > Therefore I'd like to propose this to RHEL-8.7.0. Please, provide more > justification if the decision of not fixing this for 8.6.0 has a too > negative impact. Sorry, missed this part before. The partner who reported this has a workaround (which is the right thing to do anyway), so I think we can postpone to 8.7. Thanks. Unfortunately, the patch in comment#7 cannot be used due to the fact that it is aimed at zlib-1.2.12 version. In RHEL-8 and RHEL-9 we ship zlib-1.2.11 version. Could you please specify what part of the patch you would like us to do downstream? Due to the complexity of this patch, we cannot rebase it to version 1.2.11, we need the help of the author in order to do this. ------- Comment From Rajalakshmi.Srinivasaraghavan 2022-11-14 22:31 EDT------- Manjunath from IBM is working on this. Created attachment 1928040 [details]
Patch [2/3] to add support to zlib 1.2.11
Created attachment 1928041 [details]
Patch [1/3] to add support to zlib 1.2.11
Created attachment 1928042 [details]
Patch [3/3] to add support to zlib 1.2.11
------- Comment From Manjunath.Matti 2022-11-28 10:02 EDT------- (In reply to comment #12) > Manjunath from IBM is working on this. I have attached the 3 patches, which can be applied to the zlib 1.2.11 sources. The patches are back ported from the below link. [1] https://github.com/madler/zlib/pull/478 I have built and just run make test. Looks Ok to me. ------- Comment From Manjunath.Matti 2022-11-28 10:02 EDT------- (In reply to comment #12) > Manjunath from IBM is working on this. I have attached the 3 patches, which can be applied to the zlib 1.2.11 sources. The patches are back ported from the below link. [1] https://github.com/madler/zlib/pull/478 I have built and just run make test. Looks Ok to me. Manjunath, I see you attached 3 patches targeting ppc64le. These changes will break s390x code. Is the s390x team also backporting their patches? ------- Comment From iii.com 2022-11-28 13:25 EDT------- Hi, thanks for the heads up. https://github.com/iii-i/zlib/releases/tag/crc32vx-v5 applies cleanly on top of the posted patches. My tests also pass. Best regards, Ilya Hi Ilya, This patch needs to be rebased on top of the old 1.2.11 zlib version. We don't have the latest 1.2.13 zlib version in RHEL-8, so could you please rebase it on top of the 1.2.11 version and sent it here? Thank you Gentle ping Created attachment 1941937 [details] s390x patch, rebased on top of 1.2.11 You can also find this patch at https://github.com/iii-i/zlib/commits/crc32vx-v5-1.2.11. Merged Since the problem described in this bug report should be resolved in a recent advisory, it has been closed with a resolution of ERRATA. For information on the advisory (zlib bug fix and enhancement update), and where to find the updated files, follow the link below. If the solution does not work for you, open a new bug report. https://access.redhat.com/errata/RHBA-2023:7115 The needinfo request[s] on this closed bug have been removed as they have been unresolved for 120 days |
The IFUNC resolver for crc32_z just returns a constant result on x86_64: (gdb) disassemble/m crc32_z Dump of assembler code for function crc32_z_ifunc: 261 { 0x0000000000003280 <+0>: endbr64 262 #if _ARCH_PWR8==1 263 #if defined(__BUILTIN_CPU_SUPPORTS__) 264 if (__builtin_cpu_supports("arch_2_07")) 265 return crc32_vpmsum; 266 #else 267 if (getauxval(AT_HWCAP2) & PPC_FEATURE2_ARCH_2_07) 268 return crc32_vpmsum; 269 #endif 270 #endif /* _ARCH_PWR8 */ 271 272 /* return a function pointer for optimized arches here */ 273 274 #ifdef DYNAMIC_CRC_TABLE 275 if (crc_table_empty) 276 make_crc_table(); 277 #endif /* DYNAMIC_CRC_TABLE */ 278 279 #ifdef BYFOUR 280 if (sizeof(void *) == sizeof(ptrdiff_t)) { 281 z_crc_t endian; 282 283 endian = 1; 284 if (*((unsigned char *)(&endian))) 285 return crc32_little; 0x0000000000003284 <+4>: lea -0x4eb(%rip),%rax # 0x2da0 <crc32_little> 0x000000000000328b <+11>: retq 0x000000000000328c <+0>: nopl 0x0(%rax) End of assembler dump. I assume the IFUNC resolver was added for the s390x optimizations. On x86-64, it is definitely unnecessary, likewise for AArch64. I'll check with IBM regarding the POWER8 check and file a separate bug for that if necessary. The new IFUNC resolver causes problems with symbol interposition (zlib functions are frequently bundled with other software). ELF dependency tracking in the dynamic loader does not work with interposition, and crashes can be the result. The fix is certainly to hide/rename the other symbols (so that the bundled zlib does not interpose the system implementation), but avoiding the IFUNC resolver where possible prevents these crashes and improves backwards compatibility.