From: James Simmons Date: Sat, 18 Jan 2020 15:12:38 +0000 (-0500) Subject: LU-12822 uapi: properly pack data structures X-Git-Tag: 2.13.52~75 X-Git-Url: https://git.whamcloud.com/?p=fs%2Flustre-release.git;a=commitdiff_plain;h=4751e4a951975e7fa5fa8de893224f9cc03f4165 LU-12822 uapi: properly pack data structures Linux UAPI headers use the gcc attributre __packed__ to ensure that the data structures are the exact same size on all platforms. This comes at the cost of potential misaligned accesses to these data structures which at best cost performance and at worst cause a bus error on some platforms. To detect potential misaligned access starting with gcc version 9 a new compile flags was introduced which is now impacting builds with Lustre. Examining the build failures shows most of the problems are due to packed data structures in the Lustre UAPI header containing unpacked data structure fields. Packing those missed structures resolved many of the build issues. The second problem is that the lustre utilities tend to cast some of its UAPI data structure. A good example is struct lov_user_md being cast to struct lov_user_md_v3. To ensure this is properly handled with packed data structures we need to use the __may_alias__ compiler attribute. The one exception is struct statx which is defined out side of Lustre and its unpacked. This requires extra special handling in user land code due to the described issues in this comment. Fixing this problem exposed an incorrect wiretest for struct update_op Last problem address is the use of __swabXXp() on packed data structure fields. Because of the potential alignment issues we have to use __swabXX() functions instead. Change-Id: I149c55d3361e893bd890f9c5e9c77c15f81acc1b Signed-off-by: James Simmons Reviewed-on: https://review.whamcloud.com/36798 Tested-by: jenkins Reviewed-by: Andreas Dilger Reviewed-by: Quentin Bouget Tested-by: Maloo Reviewed-by: Oleg Drokin --- diff --git a/contrib/scripts/checkpatch.pl b/contrib/scripts/checkpatch.pl index 1e9bf50..15b9dd6 100755 --- a/contrib/scripts/checkpatch.pl +++ b/contrib/scripts/checkpatch.pl @@ -5638,16 +5638,16 @@ sub process { } } -# Check for __attribute__ packed, prefer __packed +# Check for __packed, prefer __attribute__ packed if ($realfile !~ m@\binclude/uapi/@ && - $line =~ /\b__attribute__\s*\(\s*\(.*\bpacked\b/) { + $line =~ /\b__packed\b/) { WARN("PREFER_PACKED", "__packed is preferred over __attribute__((packed))\n" . $herecurr); } -# Check for __attribute__ aligned, prefer __aligned +# Check for __aligned, prefer __attribute__ aligned if ($realfile !~ m@\binclude/uapi/@ && - $line =~ /\b__attribute__\s*\(\s*\(.*aligned/) { + $line =~ /\b__aligned\b/) { WARN("PREFER_ALIGNED", "__aligned(size) is preferred over __attribute__((aligned(size)))\n" . $herecurr); } diff --git a/lustre/include/uapi/linux/lustre/lustre_idl.h b/lustre/include/uapi/linux/lustre/lustre_idl.h index 2d59f67..ab3d045 100644 --- a/lustre/include/uapi/linux/lustre/lustre_idl.h +++ b/lustre/include/uapi/linux/lustre/lustre_idl.h @@ -2740,12 +2740,12 @@ struct llog_rec_hdr { __u32 lrh_index; __u32 lrh_type; __u32 lrh_id; -}; +} __attribute__((packed)); struct llog_rec_tail { __u32 lrt_len; __u32 lrt_index; -}; +} __attribute__((packed)); /* Where data follow just after header */ #define REC_DATA(ptr) \ @@ -3325,7 +3325,7 @@ struct link_ea_entry { unsigned char lee_reclen[2]; unsigned char lee_parent_fid[sizeof(struct lu_fid)]; char lee_name[0]; -}__attribute__((packed)); +} __attribute__((packed)); /** fid2path request/reply structure */ struct getinfo_fid2path { @@ -3553,7 +3553,7 @@ struct update_op { __u16 uop_type; __u16 uop_param_count; __u16 uop_params_off[0]; -}; +} __attribute__((packed)); struct update_ops { struct update_op uops_op[0]; diff --git a/lustre/include/uapi/linux/lustre/lustre_user.h b/lustre/include/uapi/linux/lustre/lustre_user.h index 7b694fc..b893ba7 100644 --- a/lustre/include/uapi/linux/lustre/lustre_user.h +++ b/lustre/include/uapi/linux/lustre/lustre_user.h @@ -325,7 +325,7 @@ struct lu_fid { * used. **/ __u32 f_ver; -}; +} __attribute__((packed)); static inline bool fid_is_zero(const struct lu_fid *fid) { @@ -509,7 +509,7 @@ struct ost_id { } oi; struct lu_fid oi_fid; }; -}; +} __attribute__((packed)); #define DOSTID "%#llx:%llu" #define POSTID(oi) ((unsigned long long)ostid_seq(oi)), \ @@ -789,7 +789,7 @@ struct lov_user_md_v1 { /* LOV EA user data (host-endian) */ * used when reading */ }; struct lov_user_ost_data_v1 lmm_objects[0]; /* per-stripe data */ -} __attribute__((packed, __may_alias__)); +} __attribute__((packed, __may_alias__)); struct lov_user_md_v3 { /* LOV EA user data (host-endian) */ __u32 lmm_magic; /* magic number = LOV_USER_MAGIC_V3 */ @@ -805,7 +805,7 @@ struct lov_user_md_v3 { /* LOV EA user data (host-endian) */ }; char lmm_pool_name[LOV_MAXPOOLNAME + 1]; /* pool name */ struct lov_user_ost_data_v1 lmm_objects[0]; /* per-stripe data */ -} __attribute__((packed)); +} __attribute__((packed, __may_alias__)); struct lov_foreign_md { __u32 lfm_magic; /* magic number = LOV_MAGIC_FOREIGN */ @@ -813,7 +813,7 @@ struct lov_foreign_md { __u32 lfm_type; /* type, see LU_FOREIGN_TYPE_ */ __u32 lfm_flags; /* flags, type specific */ char lfm_value[]; -}; +} __attribute__((packed)); #define foreign_size(lfm) (((struct lov_foreign_md *)lfm)->lfm_length + \ offsetof(struct lov_foreign_md, lfm_value)) @@ -831,7 +831,7 @@ struct lov_foreign_md { struct lu_extent { __u64 e_start; __u64 e_end; -}; +} __attribute__((packed)); #define DEXT "[%#llx, %#llx)" #define PEXT(ext) (unsigned long long)(ext)->e_start, (unsigned long long)(ext)->e_end @@ -992,7 +992,7 @@ struct lmv_user_mds_data { struct lu_fid lum_fid; __u32 lum_padding; __u32 lum_mds; -}; +} __attribute__((packed, __may_alias__)); enum lmv_hash_type { LMV_HASH_TYPE_UNKNOWN = 0, /* 0 is reserved for testing purpose */ @@ -1645,7 +1645,7 @@ struct changelog_rec { __u32 cr_markerflags; /**< CL_MARK flags */ }; struct lu_fid cr_pfid; /**< parent fid */ -}; +} __attribute__ ((packed)); /* Changelog extension for RENAME. */ struct changelog_ext_rename { diff --git a/lustre/ptlrpc/wiretest.c b/lustre/ptlrpc/wiretest.c index b6f561d..fd14718 100644 --- a/lustre/ptlrpc/wiretest.c +++ b/lustre/ptlrpc/wiretest.c @@ -5446,7 +5446,7 @@ void lustre_assert_wire_constants(void) (long long)(int)sizeof(((struct update_params *)0)->up_params)); /* Checks for struct update_op */ - LASSERTF((int)sizeof(struct update_op) == 24, "found %lld\n", + LASSERTF((int)sizeof(struct update_op) == 20, "found %lld\n", (long long)(int)sizeof(struct update_op)); LASSERTF((int)offsetof(struct update_op, uop_fid) == 0, "found %lld\n", (long long)(int)offsetof(struct update_op, uop_fid)); diff --git a/lustre/utils/liblustreapi.c b/lustre/utils/liblustreapi.c index 96d5aa2..2129169 100644 --- a/lustre/utils/liblustreapi.c +++ b/lustre/utils/liblustreapi.c @@ -2135,12 +2135,12 @@ static int llapi_semantic_traverse(char *path, int size, DIR *parent, strcat(path, dent->d_name); if (dent->d_type == DT_UNKNOWN) { - lstatx_t *stx = ¶m->fp_lmd->lmd_stx; + struct lov_user_mds_data *lmd = param->fp_lmd; - rc = get_lmd_info(path, d, NULL, param->fp_lmd, + rc = get_lmd_info(path, d, NULL, lmd, param->fp_lum_size, GET_LMD_INFO); if (rc == 0) - dent->d_type = IFTODT(stx->stx_mode); + dent->d_type = IFTODT(lmd->lmd_stx.stx_mode); else if (ret == 0) ret = rc; @@ -3966,15 +3966,17 @@ int llapi_file_lookup(int dirfd, const char *name) * * If 0 is returned, we need to do another RPC to the OSTs to obtain the * updated timestamps. */ -static int find_time_check(lstatx_t *stx, struct find_param *param, int mds) +static int find_time_check(struct find_param *param, int mds) { + struct lov_user_mds_data *lmd = param->fp_lmd; int rc = 1; int rc2; /* Check if file is accepted. */ if (param->fp_atime) { - rc2 = find_value_cmp(stx->stx_atime.tv_sec, param->fp_atime, - param->fp_asign, param->fp_exclude_atime, + rc2 = find_value_cmp(lmd->lmd_stx.stx_atime.tv_sec, + param->fp_atime, param->fp_asign, + param->fp_exclude_atime, param->fp_time_margin, mds); if (rc2 < 0) return rc2; @@ -3982,8 +3984,9 @@ static int find_time_check(lstatx_t *stx, struct find_param *param, int mds) } if (param->fp_mtime) { - rc2 = find_value_cmp(stx->stx_mtime.tv_sec, param->fp_mtime, - param->fp_msign, param->fp_exclude_mtime, + rc2 = find_value_cmp(lmd->lmd_stx.stx_mtime.tv_sec, + param->fp_mtime, param->fp_msign, + param->fp_exclude_mtime, param->fp_time_margin, mds); if (rc2 < 0) return rc2; @@ -3995,8 +3998,9 @@ static int find_time_check(lstatx_t *stx, struct find_param *param, int mds) } if (param->fp_ctime) { - rc2 = find_value_cmp(stx->stx_ctime.tv_sec, param->fp_ctime, - param->fp_csign, param->fp_exclude_ctime, + rc2 = find_value_cmp(lmd->lmd_stx.stx_ctime.tv_sec, + param->fp_ctime, param->fp_csign, + param->fp_exclude_ctime, param->fp_time_margin, mds); if (rc2 < 0) return rc2; @@ -4010,8 +4014,9 @@ static int find_time_check(lstatx_t *stx, struct find_param *param, int mds) return rc; } -static int find_newerxy_check(lstatx_t *stx, struct find_param *param, int mds) +static int find_newerxy_check(struct find_param *param, int mds) { + struct lov_user_mds_data *lmd = param->fp_lmd; int i; int rc = 1; int rc2; @@ -4019,7 +4024,7 @@ static int find_newerxy_check(lstatx_t *stx, struct find_param *param, int mds) for (i = 0; i < 2; i++) { /* Check if file is accepted. */ if (param->fp_newery[NEWERXY_ATIME][i]) { - rc2 = find_value_cmp(stx->stx_atime.tv_sec, + rc2 = find_value_cmp(lmd->lmd_stx.stx_atime.tv_sec, param->fp_newery[NEWERXY_ATIME][i], -1, i, 0, mds); if (rc2 < 0) @@ -4028,7 +4033,7 @@ static int find_newerxy_check(lstatx_t *stx, struct find_param *param, int mds) } if (param->fp_newery[NEWERXY_MTIME][i]) { - rc2 = find_value_cmp(stx->stx_mtime.tv_sec, + rc2 = find_value_cmp(lmd->lmd_stx.stx_mtime.tv_sec, param->fp_newery[NEWERXY_MTIME][i], -1, i, 0, mds); if (rc2 < 0) @@ -4043,7 +4048,7 @@ static int find_newerxy_check(lstatx_t *stx, struct find_param *param, int mds) } if (param->fp_newery[NEWERXY_CTIME][i]) { - rc2 = find_value_cmp(stx->stx_ctime.tv_sec, + rc2 = find_value_cmp(lmd->lmd_stx.stx_ctime.tv_sec, param->fp_newery[NEWERXY_CTIME][i], -1, i, 0, mds); if (rc2 < 0) @@ -4070,14 +4075,14 @@ static int check_obd_match(struct find_param *param) { struct lov_user_ost_data_v1 *objects; struct lov_comp_md_v1 *comp_v1 = NULL; - struct lov_user_md_v1 *v1 = ¶m->fp_lmd->lmd_lmm; - lstatx_t *stx = ¶m->fp_lmd->lmd_stx; + struct lov_user_mds_data *lmd = param->fp_lmd; + struct lov_user_md_v1 *v1 = &lmd->lmd_lmm; int i, j, k, count = 1; if (param->fp_obd_uuid && param->fp_obd_index == OBD_NOT_FOUND) return 0; - if (!S_ISREG(stx->stx_mode)) + if (!S_ISREG(lmd->lmd_stx.stx_mode)) return 0; /* exclude foreign */ @@ -4387,9 +4392,9 @@ static int find_check_pool(struct find_param *param) static int find_check_comp_options(struct find_param *param) { - lstatx_t *stx = ¶m->fp_lmd->lmd_stx; struct lov_comp_md_v1 *comp_v1, *forged_v1 = NULL; - struct lov_user_md_v1 *v1 = ¶m->fp_lmd->lmd_lmm; + struct lov_user_mds_data *lmd = param->fp_lmd; + struct lov_user_md_v1 *v1 = &lmd->lmd_lmm; struct lov_comp_md_entry_v1 *entry; int i, ret = 0; @@ -4405,7 +4410,8 @@ static int find_check_comp_options(struct find_param *param) comp_v1 = forged_v1; comp_v1->lcm_entry_count = 1; entry = &comp_v1->lcm_entries[0]; - entry->lcme_flags = S_ISDIR(stx->stx_mode) ? 0 : LCME_FL_INIT; + entry->lcme_flags = S_ISDIR(lmd->lmd_stx.stx_mode) ? + 0 : LCME_FL_INIT; entry->lcme_extent.e_start = 0; entry->lcme_extent.e_end = LUSTRE_EOF; } @@ -4531,9 +4537,9 @@ static int cb_find_init(char *path, DIR *parent, DIR **dirp, void *data, struct dirent64 *de) { struct find_param *param = (struct find_param *)data; + struct lov_user_mds_data *lmd = param->fp_lmd; DIR *dir = dirp == NULL ? NULL : *dirp; int decision = 1; /* 1 is accepted; -1 is rejected. */ - lstatx_t *stx = ¶m->fp_lmd->lmd_stx; int lustre_fs = 1; int checked_type = 0; int ret = 0; @@ -4623,7 +4629,7 @@ static int cb_find_init(char *path, DIR *parent, DIR **dirp, if (dir != NULL) { ret = llapi_file_fget_mdtidx(dirfd(dir), ¶m->fp_file_mdt_index); - } else if (S_ISREG(stx->stx_mode)) { + } else if (S_ISREG(lmd->lmd_stx.stx_mode)) { /* FIXME: we could get the MDT index from the * file's FID in lmd->lmd_lmm.lmm_oi without * opening the file, once we are sure that @@ -4657,7 +4663,7 @@ static int cb_find_init(char *path, DIR *parent, DIR **dirp, } if (param->fp_type && !checked_type) { - if ((stx->stx_mode & S_IFMT) == param->fp_type) { + if ((lmd->lmd_stx.stx_mode & S_IFMT) == param->fp_type) { if (param->fp_exclude_type) goto decided; } else { @@ -4669,8 +4675,8 @@ static int cb_find_init(char *path, DIR *parent, DIR **dirp, /* Prepare odb. */ if (param->fp_obd_uuid || param->fp_mdt_uuid) { if (lustre_fs && param->fp_got_uuids && - param->fp_dev != makedev(stx->stx_dev_major, - stx->stx_dev_minor)) { + param->fp_dev != makedev(lmd->lmd_stx.stx_dev_major, + lmd->lmd_stx.stx_dev_minor)) { /* A lustre/lustre mount point is crossed. */ param->fp_got_uuids = 0; param->fp_obds_printed = 0; @@ -4684,8 +4690,8 @@ static int cb_find_init(char *path, DIR *parent, DIR **dirp, if (ret) goto out; - param->fp_dev = makedev(stx->stx_dev_major, - stx->stx_dev_minor); + param->fp_dev = makedev(lmd->lmd_stx.stx_dev_major, + lmd->lmd_stx.stx_dev_minor); } else if (!lustre_fs && param->fp_got_uuids) { /* A lustre/non-lustre mount point is crossed. */ param->fp_got_uuids = 0; @@ -4787,7 +4793,7 @@ static int cb_find_init(char *path, DIR *parent, DIR **dirp, obd_matches: if (param->fp_check_uid) { - if (stx->stx_uid == param->fp_uid) { + if (lmd->lmd_stx.stx_uid == param->fp_uid) { if (param->fp_exclude_uid) goto decided; } else { @@ -4797,7 +4803,7 @@ obd_matches: } if (param->fp_check_gid) { - if (stx->stx_gid == param->fp_gid) { + if (lmd->lmd_stx.stx_gid == param->fp_gid) { if (param->fp_exclude_gid) goto decided; } else { @@ -4852,8 +4858,8 @@ obd_matches: int for_mds; for_mds = lustre_fs ? - (S_ISREG(stx->stx_mode) && stripe_count) : 0; - decision = find_time_check(stx, param, for_mds); + (S_ISREG(lmd->lmd_stx.stx_mode) && stripe_count) : 0; + decision = find_time_check(param, for_mds); if (decision == -1) goto decided; } @@ -4862,23 +4868,23 @@ obd_matches: int for_mds; for_mds = lustre_fs ? - (S_ISREG(stx->stx_mode) && stripe_count) : 0; - decision = find_newerxy_check(stx, param, for_mds); + (S_ISREG(lmd->lmd_stx.stx_mode) && stripe_count) : 0; + decision = find_newerxy_check(param, for_mds); if (decision == -1) goto decided; } flags = param->fp_lmd->lmd_flags; if (param->fp_check_size && - ((S_ISREG(stx->stx_mode) && stripe_count) || - S_ISDIR(stx->stx_mode)) && + ((S_ISREG(lmd->lmd_stx.stx_mode) && stripe_count) || + S_ISDIR(lmd->lmd_stx.stx_mode)) && !(flags & OBD_MD_FLSIZE || (param->fp_lazy && flags & OBD_MD_FLLAZYSIZE))) decision = 0; if (param->fp_check_blocks && - ((S_ISREG(stx->stx_mode) && stripe_count) || - S_ISDIR(stx->stx_mode)) && + ((S_ISREG(lmd->lmd_stx.stx_mode) && stripe_count) || + S_ISDIR(lmd->lmd_stx.stx_mode)) && !(flags & OBD_MD_FLBLOCKS || (param->fp_lazy && flags & OBD_MD_FLLAZYBLOCKS))) decision = 0; @@ -4923,12 +4929,12 @@ obd_matches: convert_lmd_statx(param->fp_lmd, &st, true); /* Check the time on osc. */ - decision = find_time_check(stx, param, 0); + decision = find_time_check(param, 0); if (decision == -1) goto decided; if (param->fp_newerxy) { - decision = find_newerxy_check(stx, param, 0); + decision = find_newerxy_check(param, 0); if (decision == -1) goto decided; @@ -4936,7 +4942,8 @@ obd_matches: } if (param->fp_check_size) { - decision = find_value_cmp(stx->stx_size, param->fp_size, + decision = find_value_cmp(lmd->lmd_stx.stx_size, + param->fp_size, param->fp_size_sign, param->fp_exclude_size, param->fp_size_units, 0); @@ -4945,7 +4952,7 @@ obd_matches: } if (param->fp_check_blocks) { /* convert st_blocks to bytes */ - decision = find_value_cmp(stx->stx_blocks * 512, + decision = find_value_cmp(lmd->lmd_stx.stx_blocks * 512, param->fp_blocks, param->fp_blocks_sign, param->fp_exclude_blocks, diff --git a/lustre/utils/liblustreapi_layout.c b/lustre/utils/liblustreapi_layout.c index e6ad5fd..c9525fe 100644 --- a/lustre/utils/liblustreapi_layout.c +++ b/lustre/utils/liblustreapi_layout.c @@ -135,11 +135,11 @@ llapi_layout_swab_lov_user_md(struct lov_user_md *lum, int lum_size) comp_v1 = (struct lov_comp_md_v1 *)lum; if (comp_v1 != NULL) { - __swab32s(&comp_v1->lcm_magic); - __swab32s(&comp_v1->lcm_size); - __swab32s(&comp_v1->lcm_layout_gen); - __swab16s(&comp_v1->lcm_flags); - __swab16s(&comp_v1->lcm_entry_count); + comp_v1->lcm_magic = __swab32(comp_v1->lcm_magic); + comp_v1->lcm_size = __swab32(comp_v1->lcm_size); + comp_v1->lcm_layout_gen = __swab32(comp_v1->lcm_layout_gen); + comp_v1->lcm_flags = __swab16(comp_v1->lcm_flags); + comp_v1->lcm_entry_count = __swab16(comp_v1->lcm_entry_count); ent_count = comp_v1->lcm_entry_count; } else { ent_count = 1; @@ -148,13 +148,13 @@ llapi_layout_swab_lov_user_md(struct lov_user_md *lum, int lum_size) for (i = 0; i < ent_count; i++) { if (comp_v1 != NULL) { ent = &comp_v1->lcm_entries[i]; - __swab32s(&ent->lcme_id); - __swab32s(&ent->lcme_flags); - __swab64s(&ent->lcme_timestamp); - __swab64s(&ent->lcme_extent.e_start); - __swab64s(&ent->lcme_extent.e_end); - __swab32s(&ent->lcme_offset); - __swab32s(&ent->lcme_size); + ent->lcme_id = __swab32(ent->lcme_id); + ent->lcme_flags = __swab32(ent->lcme_flags); + ent->lcme_timestamp = __swab64(ent->lcme_timestamp); + ent->lcme_extent.e_start = __swab64(ent->lcme_extent.e_start); + ent->lcme_extent.e_end = __swab64(ent->lcme_extent.e_end); + ent->lcme_offset = __swab32(ent->lcme_offset); + ent->lcme_size = __swab32(ent->lcme_size); lum = (struct lov_user_md *)((char *)comp_v1 + ent->lcme_offset); @@ -162,11 +162,11 @@ llapi_layout_swab_lov_user_md(struct lov_user_md *lum, int lum_size) } obj_count = llapi_layout_objects_in_lum(lum, lum_size); - __swab32s(&lum->lmm_magic); - __swab32s(&lum->lmm_pattern); - __swab32s(&lum->lmm_stripe_size); - __swab16s(&lum->lmm_stripe_count); - __swab16s(&lum->lmm_stripe_offset); + lum->lmm_magic = __swab32(lum->lmm_magic); + lum->lmm_pattern = __swab32(lum->lmm_pattern); + lum->lmm_stripe_size = __swab32(lum->lmm_stripe_size); + lum->lmm_stripe_count = __swab16(lum->lmm_stripe_count); + lum->lmm_stripe_offset = __swab16(lum->lmm_stripe_offset); if (lum->lmm_magic != LOV_MAGIC_V1) { struct lov_user_md_v3 *v3; @@ -177,7 +177,7 @@ llapi_layout_swab_lov_user_md(struct lov_user_md *lum, int lum_size) } for (j = 0; j < obj_count; j++) - __swab32s(&lod[j].l_ost_idx); + lod[j].l_ost_idx = __swab32(lod[j].l_ost_idx); } } diff --git a/lustre/utils/wiretest.c b/lustre/utils/wiretest.c index 4940411..391ca56 100644 --- a/lustre/utils/wiretest.c +++ b/lustre/utils/wiretest.c @@ -5476,7 +5476,7 @@ void lustre_assert_wire_constants(void) (long long)(int)sizeof(((struct update_params *)0)->up_params)); /* Checks for struct update_op */ - LASSERTF((int)sizeof(struct update_op) == 24, "found %lld\n", + LASSERTF((int)sizeof(struct update_op) == 20, "found %lld\n", (long long)(int)sizeof(struct update_op)); LASSERTF((int)offsetof(struct update_op, uop_fid) == 0, "found %lld\n", (long long)(int)offsetof(struct update_op, uop_fid));