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

Linux 6.3 compat: Fix memcpy "detected field-spanning write" error #14737

Merged
merged 2 commits into from
Apr 13, 2023

Conversation

youzhongyang
Copy link
Contributor

@youzhongyang youzhongyang commented Apr 11, 2023

Signed-off-by: Youzhong Yang yyang@mathworks.com

Motivation and Context

The following errors were observed when running zfs test suite:

[ 1667.309351] memcpy: detected field-spanning write (size 3904) of single field "((void*)((dn->dn_phys)->dn_bonus + (((dn->dn_phys)->dn_nblkptr - 1) * sizeof (blkptr_t))))" at /tmp/zfs-build-root-YvJzdwOd/BUILD/zfs-kmod-2.1.99/_kmod_build_6.3.0-0.rc5.20230405git76f598ba7d8e.44.fc39.x86_64/../zfs-2.1.99/module/zfs/dbuf.c:4112 (size 320)
[ 1667.357445] memcpy: detected field-spanning write (size 3904) of single field "((void*)((&ddnp[i])->dn_bonus + (((&ddnp[i])->dn_nblkptr - 1) * sizeof (blkptr_t))))" at /tmp/zfs-build-root-YvJzdwOd/BUILD/zfs-kmod-2.1.99/_kmod_build_6.3.0-0.rc5.20230405git76f598ba7d8e.44.fc39.x86_64/../zfs-2.1.99/module/os/linux/zfs/zio_crypt.c:902 (size 320)

The fortified memcpy() and memmove() starting from Linux kernel 6.1+ will generate the above warning message when copying bonus data of more than 320 bytes into the dnode_phys_t.

The error could be reproduced by /usr/share/zfs/zfs-tests.sh -T zfs_diff.

Description

Add a new union member of flexible array to dnode_phys_t and use it in the macro so we can silence the memcpy() fortify error.

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@rincebrain
Copy link
Contributor

Mightn't it be better to do a define for which function to use and call that define, so that we don't have to remember to update both sides of the ifdef every time going forward?

Signed-off-by: Youzhong Yang <yyang@mathworks.com>
@youzhongyang
Copy link
Contributor Author

I looked at the Linux kernel side of changes, most of them took the approach of changing struct definition.

I thought about it, this is likely the best way to address this issue. I see no harm adding a new union member, if you have any objection, please let me know.

@rincebrain
Copy link
Contributor

I'd recommend at least a comment explaining why that's necessary, if you're going to do it that way.

Signed-off-by: Youzhong Yang <yyang@mathworks.com>
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Apr 11, 2023
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Apr 13, 2023
@behlendorf behlendorf merged commit 27a82cb into openzfs:master Apr 13, 2023
@pjd
Copy link
Contributor

pjd commented Apr 14, 2023

This doesn't compile on FreeBSD (with clang):

In file included from /root/github/pjd/openzfs/module/os/freebsd/zfs/dmu_os.c:39:
/root/github/pjd/openzfs/include/sys/dmu_objset.h:76:15: error: field 'os_meta_dnode' with variable sized type 'dnode_phys_t' (aka 'struct dnode_phys') not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
dnode_phys_t os_meta_dnode;
^
/root/github/pjd/openzfs/include/sys/dmu_objset.h:85:15: error: field 'os_userused_dnode' with variable sized type 'dnode_phys_t' (aka 'struct dnode_phys') not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
dnode_phys_t os_userused_dnode;
^
/root/github/pjd/openzfs/include/sys/dmu_objset.h:86:15: error: field 'os_groupused_dnode' with variable sized type 'dnode_phys_t' (aka 'struct dnode_phys') not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
dnode_phys_t os_groupused_dnode;
^
/root/github/pjd/openzfs/include/sys/dmu_objset.h:87:15: error: field 'os_projectused_dnode' with variable sized type 'dnode_phys_t' (aka 'struct dnode_phys') not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
dnode_phys_t os_projectused_dnode;
^
4 errors generated.
*** Error code 1

@behlendorf
Copy link
Contributor

I see no harm adding a new union member, if you have any objection, please let me know.

Unfortunately the FreeBSD builder was offline and we failed to notice this does introduce a build failure on FreeBSD. We'll either need to revert this for now, or wrap this GNU extension in a __GNUC__ guard. @youzhongyang do you happen to know why similar changes made to Linux didn't hit this as well? Do they simply disable the warrning?

@youzhongyang
Copy link
Contributor Author

I see no harm adding a new union member, if you have any objection, please let me know.

Unfortunately the FreeBSD builder was offline and we failed to notice this does introduce a build failure on FreeBSD. We'll either need to revert this for now, or wrap this GNU extension in a __GNUC__ guard. @youzhongyang do you happen to know why similar changes made to Linux didn't hit this as well? Do they simply disable the warrning?

I looked at Linux kernel src a bit and didn't find anything special. I've created a PR following your suggestion.

I am on the road and will do more research about this clang flexible array thing. If someone has better idea tackling this issue, please advise in the PR.

andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Apr 30, 2023
Add a new union member of flexible array to dnode_phys_t and use
it in the macro so we can silence the memcpy() fortify error.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Youzhong Yang <yyang@mathworks.com>
Closes openzfs#14737
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Apr 30, 2023
Add a new union member of flexible array to dnode_phys_t and use
it in the macro so we can silence the memcpy() fortify error.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Youzhong Yang <yyang@mathworks.com>
Closes openzfs#14737
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 2, 2023
Add a new union member of flexible array to dnode_phys_t and use
it in the macro so we can silence the memcpy() fortify error.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Youzhong Yang <yyang@mathworks.com>
Closes openzfs#14737
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 2, 2023
Add a new union member of flexible array to dnode_phys_t and use
it in the macro so we can silence the memcpy() fortify error.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Youzhong Yang <yyang@mathworks.com>
Closes openzfs#14737
tonyhutter pushed a commit that referenced this pull request Jun 5, 2023
Add a new union member of flexible array to dnode_phys_t and use
it in the macro so we can silence the memcpy() fortify error.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Youzhong Yang <yyang@mathworks.com>
Closes #14737
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants