On 12/07/2023 12:20, Kevin Brodsky wrote:
On 08/06/2023 10:42, Zachary Leaf wrote:
union bpf_attr member info.info is a __user *ptr to either: struct bpf_btf_info struct bpf_prog_info struct bpf_map_info struct bpf_link_info
These info structs are passed in from userspace and used to store information returned from calls to BPF_OBJ_GET_INFO_BY_FD.
While inbound conversion for all other members of union bpf_attr is handled upfront, this same approach is not possible here for two main reasons:
Due to the extra complexity of a further userspace struct that is one of 4 types. These types themselves can be a union/multiplex of even more additional options and types. It is not straight forward or possible in many cases to determine which elements of the union are active upfront, and each has different native/compat64 layouts/offsets.
Due to the nature of this subcommand, the main purpose is to copy information back out to userspace, there is more work to do to convert the struct back out to compat64 compatible offsets compared to other subcommands.
Instead of an upfront conversion, convert where each is used at the end site both in/out.
The commit message is helpful, thanks, but having exactly the same one in patch 5-8 is less so. It begs the question of whether it really makes sense for these patches to be separate. I realise that merging them would create a big diff, but considering that each patch does pretty much the same thing for each struct there is not much complexity involved, and having it all in one patch would be consistent.
No problem. As per the cover letter, really it was just to make it easier to review. I agree these bpf_xyz_info can be merged, especially now that bpf_map_info patch has been removed.
Signed-off-by: Zachary Leaf zachary.leaf@arm.com
include/linux/bpf_compat.h | 9 ++++ kernel/bpf/btf.c | 91 +++++++++++++++++++++++++++++++++++--- kernel/bpf/syscall.c | 8 ---- 3 files changed, 93 insertions(+), 15 deletions(-)
diff --git a/include/linux/bpf_compat.h b/include/linux/bpf_compat.h index 3d5f6bd2cd46..6882322cefc2 100644 --- a/include/linux/bpf_compat.h +++ b/include/linux/bpf_compat.h @@ -287,5 +287,14 @@ union compat_bpf_attr { } __attribute__((aligned(8))); +struct compat_bpf_btf_info {
- __aligned_u64 btf;
- __u32 btf_size;
- __u32 id;
- __aligned_u64 name;
- __u32 name_len;
- __u32 kernel_btf;
+} __attribute__((aligned(8)));
#endif /* CONFIG_COMPAT64 */ diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 35c07afac924..25cb5231f4df 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -16,6 +16,7 @@ #include <linux/kernel.h> #include <linux/idr.h> #include <linux/sort.h> +#include <linux/bpf_compat.h> #include <linux/bpf_verifier.h> #include <linux/btf.h> #include <linux/btf_ids.h> @@ -6942,6 +6943,30 @@ struct btf *btf_get_by_fd(int fd) return btf; } +#ifdef CONFIG_COMPAT64 +void convert_compat_btf_info_in(struct bpf_btf_info *dest,
struct compat_bpf_btf_info *cinfo)Nit: the input pointer could be const. Same in similar conversion functions.
Ack
+{
- dest->btf = cinfo->btf;
- dest->btf_size = cinfo->btf_size;
- dest->id = cinfo->id;
- dest->name = cinfo->name;
- dest->name_len = cinfo->name_len;
- dest->kernel_btf = cinfo->kernel_btf;
+}
+void convert_compat_btf_info_out(struct compat_bpf_btf_info *dest,
struct bpf_btf_info *info)+{
- dest->btf = info->btf;
- dest->btf_size = info->btf_size;
- dest->id = info->id;
- dest->name = info->name;
- dest->name_len = info->name_len;
- dest->kernel_btf = info->kernel_btf;
+} +#endif /* CONFIG_COMPAT64 */
int btf_get_info_by_fd(const struct btf *btf, const union bpf_attr *attr, union bpf_attr __user *uattr) @@ -6953,15 +6978,56 @@ int btf_get_info_by_fd(const struct btf *btf, char __user *uname; u32 uinfo_len, uname_len, name_len; int ret = 0; +#ifdef CONFIG_COMPAT64
- struct compat_bpf_btf_info __user *cuinfo;
- struct compat_bpf_btf_info cinfo;
- union compat_bpf_attr __user *cuattr =
(union compat_bpf_attr __user *)uattr;- u32 cuinfo_len;
- uinfo = u64_to_user_ptr(attr->info.info);
- uinfo_len = attr->info.info_len;
- /*
* for compat64, uattr has already been converted and copied into* attr - however attr->info.info is a ptr to uapi struct bpf_btf_info,* which also needs converting** convert bpf_btf_info coming in, and later reverse this conversion* when writing back out to userspace*/- if (in_compat_syscall()) {
cuinfo = (struct compat_bpf_btf_info __user *)attr->info.info;This is not a valid cast at this stage, as info.info is still a 64-bit integer. I'd just use u64_to_user_ptr() and remove it in the final patch like the others.
Ack
cuinfo_len = attr->info.info_len;
- info_copy = min_t(u32, uinfo_len, sizeof(info));
- memset(&info, 0, sizeof(info));
- if (copy_from_user(&info, uinfo, info_copy))
return -EFAULT;
ret = bpf_check_uarg_tail_zero(USER_BPFPTR(cuinfo),sizeof(cinfo), cuinfo_len);if (ret)return ret;info_copy = min_t(u32, cuinfo_len, sizeof(cinfo));if (copy_from_user_with_ptr(&cinfo, cuinfo, info_copy))return -EFAULT;memset(&info, 0, sizeof(info));convert_compat_btf_info_in(&info, &cinfo);- } else
+#endif
- {
uinfo = u64_to_user_ptr(attr->info.info);uinfo_len = attr->info.info_len;ret = bpf_check_uarg_tail_zero(USER_BPFPTR(uinfo),sizeof(info), uinfo_len);if (ret)return ret;
info_copy = min_t(u32, uinfo_len, sizeof(info));memset(&info, 0, sizeof(info));if (copy_from_user_with_ptr(&info, uinfo, info_copy))return -EFAULT;- }
- /*
* struct btf found in kernel/bpf/btf.c is kernel only* copy relevant fields into bpf_btf_info, no need to convert* anything in compat64 case*/Not sure this comment is adding much, it doesn't make much difference that the data comes from struct btf or somewhere else.
Ack
Kevin
info.id = btf->id; ubtf = u64_to_user_ptr(info.btf); btf_copy = min_t(u32, btf->data_size, info.btf_size); @@ -6995,7 +7061,18 @@ int btf_get_info_by_fd(const struct btf *btf, } }
- if (copy_to_user(uinfo, &info, info_copy) ||
+#ifdef CONFIG_COMPAT64
- if (in_compat_syscall()) {
convert_compat_btf_info_out(&cinfo, &info);if (copy_to_user_with_ptr(cuinfo, &cinfo, info_copy))return -EFAULT;if (put_user(info_copy, &cuattr->info.info_len))return -EFAULT;return ret;- }
+#endif
- if (copy_to_user_with_ptr(uinfo, &info, info_copy) || put_user(info_copy, &uattr->info.info_len)) return -EFAULT;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 4c86b1b23f36..d06700136e1f 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -4230,14 +4230,6 @@ static int bpf_btf_get_info_by_fd(struct file *file, const union bpf_attr *attr, union bpf_attr __user *uattr) {
- struct bpf_btf_info __user *uinfo = u64_to_user_ptr(attr->info.info);
- u32 info_len = attr->info.info_len;
- int err;
- err = bpf_check_uarg_tail_zero(USER_BPFPTR(uinfo), sizeof(*uinfo), info_len);
- if (err)
return err;- return btf_get_info_by_fd(btf, attr, uattr);
}