Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Deepin-Kernel-SIG] crypto: mips/crc32 - fix the CRC32C implementation #452

Merged

Conversation

Avenger-285714
Copy link
Collaborator

Commit ca459e5f826f ("crypto: mips/crc32 - Clean up useless assignment operations") changed crc32c_mips_le_hw() to use the instructions that use the "regular" CRC32 polynomial instead of the Castagnoli polynomial. Therefore it can't be computing CRC32C values correctly anymore.

I haven't been successful in running a MIPS kernel in QEMU, but based on code review this is the fix that is needed.

Fixes: ca459e5f826f ("crypto: mips/crc32 - Clean up useless assignment operations")
Cc: Guan Wentao guanwentao@uniontech.com
Cc: WangYuli wangyuli@uniontech.com

Acked-by: Wentao Guan guanwentao@uniontech.com
Acked-by: WangYuli wangyuli@uniontech.com

Commit ca459e5f826f ("crypto: mips/crc32 - Clean up useless assignment
operations") changed crc32c_mips_le_hw() to use the instructions that
use the "regular" CRC32 polynomial instead of the Castagnoli polynomial.
Therefore it can't be computing CRC32C values correctly anymore.

I haven't been successful in running a MIPS kernel in QEMU, but based on
code review this is the fix that is needed.

Fixes: ca459e5f826f ("crypto: mips/crc32 - Clean up useless assignment operations")
Cc: Guan Wentao <guanwentao@uniontech.com>
Cc: WangYuli <wangyuli@uniontech.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
Acked-by: Wentao Guan <guanwentao@uniontech.com>
Acked-by: WangYuli <wangyuli@uniontech.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
@deepin-ci-robot
Copy link

deepin pr auto review

代码审查意见:

  1. 函数名更改:将 CRC32 更改为 CRC32C 可能会影响其他依赖 CRC32 的代码部分。需要确认这一更改是否已在整个项目中得到一致的应用,并且所有相关的调用都已经更新。

  2. 宏定义CRC32CRC32C 是宏定义,用于计算不同的 CRC32 校验和。如果 CRC32C 宏的实现与 CRC32 不同,那么这一更改可能会引入逻辑错误。需要确保 CRC32C 宏的正确性和与 CRC32 的兼容性。

  3. 代码可读性:代码中的注释和变量命名应该清晰,以便其他开发者理解代码的意图。如果 CRC32CRC32C 的实现不同,应该在代码中添加相应的注释说明。

  4. 性能考虑:如果 CRC32C 的实现比 CRC32 更快,那么这一更改是有益的。如果 CRC32C 的实现更慢,那么需要评估这一更改对性能的影响。

  5. 错误处理:代码中没有错误处理机制。如果 get_unaligned_le64get_unaligned_le32 函数失败,应该有相应的错误处理逻辑。

  6. 边界条件:代码中没有处理 len 为 0 的情况。应该添加对 len 为 0 的检查,以避免不必要的循环。

  7. 代码风格:代码风格应该保持一致,例如缩进和空格的使用。如果项目中有一个统一的代码风格指南,应该遵循该指南。

综上所述,需要确保 CRC32C 的实现是正确的,并且与 CRC32 兼容。同时,需要添加适当的错误处理和边界条件检查,并保持代码风格的一致性。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from avenger-285714. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Avenger-285714 Avenger-285714 merged commit dc8f26e into deepin-community:linux-6.6.y Nov 2, 2024
3 of 5 checks passed
Avenger-285714 pushed a commit to Avenger-285714/DeepinKernel that referenced this pull request Jan 8, 2025
[ Upstream commit 2fd0eba ]

commit c35559f ("x86/shstk: Introduce map_shadow_stack syscall")
recently added support for map_shadow_stack() but it is limited to x86
only for now. There is a possibility that other architectures (namely,
arm64 and RISC-V), that are implementing equivalent support for shadow
stacks, might need to add support for it.

Independent of that, reserving arch-specific syscall numbers in the
syscall tables of all architectures is good practice and would help
avoid future conflicts. map_shadow_stack() is marked as a conditional
syscall in sys_ni.c. Adding it to the syscall tables of other
architectures is harmless and would return ENOSYS when exercised.

Note, map_shadow_stack() was assigned deepin-community#453 during the merge process
since deepin-community#452 was taken by fchmodat2().

For Powerpc, map it to sys_ni_syscall() as is the norm for Powerpc
syscall tables.

For Alpha, map_shadow_stack() takes up deepin-community#563 as Alpha still diverges from
the common syscall numbering system in the other architectures.

Link: https://lore.kernel.org/lkml/20230515212255.GA562920@debug.ba.rivosinc.com/
Link: https://lore.kernel.org/lkml/b402b80b-a7c6-4ef0-b977-c0f5f582b78a@sirena.org.uk/

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
[ deepin kernel ]
Signed-off-by: WangYuli <wangyuli@uniontech.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants