On Sat, Nov 25, 2017 at 05:59:54PM -0800, Alexei Starovoitov wrote:
> If we were poking into 'struct perf_event_attr __user *uptr'
> directly like get|put_user(.., &uptr->config)
> then 32-bit user space with 4-byte aligned u64s would cause
> 64-bit kernel to trap on archs like sparc.
But surely archs that have hardware alignment requirements have __u64 ==
__aligned_u64 ?
It's just that the structure layout can change between archs that have
__u64 != __aligned_u64 and __u64 == __aligned_u64.
But I would argue an architecture that has hardware alignment
requirements and has an unaligned __u64 is just plain broken.
> But in this case you're right. We can use config[12] as-is, since these
> u64 fields are passing the value one way only (into the kernel) and
> we do full perf_copy_attr() first and all further accesses are from
> copied structure and u64_to_user_ptr(event->attr.config) will be fine.
Right. Also note that there are no holes in perf_event_attr, if the
structure itself is allocated aligned the individual fields will be
aligned.
> Do you mind we do
> union {
> __u64 file_path;
> __u64 func_name;
> __u64 config;
> };
> and similar with config1 ?
> Or prefer that we use 'config/config1' to store string+offset there?
> I think config/config1 is cleaner than config1/config2
I would prefer you use config1/config2 for this and leave config itself
for modifiers (like the retprobe thing). It also better lines up with
the BP stuff.
A little something like so perhaps:
diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index 362493a2f950..b6e76512f757 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -380,10 +380,14 @@ struct perf_event_attr {
__u32 bp_type;
union {
__u64 bp_addr;
+ __u64 uprobe_path;
+ __u64 kprobe_func;
__u64 config1; /* extension of config */
};
union {
__u64 bp_len;
+ __u64 kprobe_addr;
+ __u64 probe_offset;
__u64 config2; /* extension of config1 */
};
__u64 branch_sample_type; /* enum perf_branch_sample_type */
