PERF_EVENT_IOC_SET_FILTER has different values based on bitness

PERF_EVENT_IOC_SET_FILTER has different values based on bitness

[David Ahern]

32-bit perf binaries are not able to set filters on 64-bit kernels.

    $ perf record -e  net:netif_receive_skb --filter 'name == "eth1"
    Error: failed to set filter with 25 (Inappropriate ioctl for device)

The reason is that the definition of PERF_EVENT_IOC_SET_FILTER contains 
a pointer:

#define PERF_EVENT_IOC_SET_FILTER       _IOW('$', 6, char *)

the size of which of course differs for 32-bit and 64-bit. This has been 
there since the original commit (6fb2915df7f07) back in 2009.

Thoughts on how to fix this? Changing the definition of SET_FILTER 
breaks existing setups so that rules it out. What about something like this:

#define PERF_EVENT_IOC_SET_FILTER_32       _IOW('$', 6, u32)

and then

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 963bf139e2b2..c805132ac1cf 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3714,6 +3714,7 @@ static long perf_ioctl(struct file *file, unsigned 
int cmd, unsigned long arg)
     }

     case PERF_EVENT_IOC_SET_FILTER:
+   case PERF_EVENT_IOC_SET_FILTER_32:
         return perf_event_set_filter(event, (void __user *)arg);

     default:

David

Re: PERF_EVENT_IOC_SET_FILTER has different values based on bitness

[David Ahern]

On 10/7/14, 5:07 PM, David Ahern wrote:
>
> 32-bit perf binaries are not able to set filters on 64-bit kernels.
>
>     $ perf record -e  net:netif_receive_skb --filter 'name == "eth1"
>     Error: failed to set filter with 25 (Inappropriate ioctl for device)
>
> The reason is that the definition of PERF_EVENT_IOC_SET_FILTER contains
> a pointer:
>
> #define PERF_EVENT_IOC_SET_FILTER       _IOW('$', 6, char *)
>
> the size of which of course differs for 32-bit and 64-bit. This has been
> there since the original commit (6fb2915df7f07) back in 2009.
>
> Thoughts on how to fix this? Changing the definition of SET_FILTER
> breaks existing setups so that rules it out. What about something like
> this:
>
> #define PERF_EVENT_IOC_SET_FILTER_32       _IOW('$', 6, u32)
>
> and then
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 963bf139e2b2..c805132ac1cf 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -3714,6 +3714,7 @@ static long perf_ioctl(struct file *file, unsigned
> int cmd, unsigned long arg)
>      }
>
>      case PERF_EVENT_IOC_SET_FILTER:
> +   case PERF_EVENT_IOC_SET_FILTER_32:
>          return perf_event_set_filter(event, (void __user *)arg);
>
>      default:
>

Oh, PERF_EVENT_IOC_ID has the same problem:

#define PERF_EVENT_IOC_ID               _IOR('$', 7, __u64 *)

David

Re: PERF_EVENT_IOC_SET_FILTER has different values based on bitness

[Andi Kleen]

David Ahern <dsahern@gmail.com> writes:
>>
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index 963bf139e2b2..c805132ac1cf 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -3714,6 +3714,7 @@ static long perf_ioctl(struct file *file, unsigned
>> int cmd, unsigned long arg)
>>      }
>>
>>      case PERF_EVENT_IOC_SET_FILTER:
>> +   case PERF_EVENT_IOC_SET_FILTER_32:
>>          return perf_event_set_filter(event, (void __user *)arg);
>>
>>      default:
>>
>
> Oh, PERF_EVENT_IOC_ID has the same problem:
>
> #define PERF_EVENT_IOC_ID               _IOR('$', 7, __u64 *)

The right way is to add a compat_perf_ioctl() 

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: PERF_EVENT_IOC_SET_FILTER has different values based on bitness

[David Ahern]

On 10/7/14, 6:50 PM, Andi Kleen wrote:
> David Ahern <dsahern@gmail.com> writes:
>>>
>>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>>> index 963bf139e2b2..c805132ac1cf 100644
>>> --- a/kernel/events/core.c
>>> +++ b/kernel/events/core.c
>>> @@ -3714,6 +3714,7 @@ static long perf_ioctl(struct file *file, unsigned
>>> int cmd, unsigned long arg)
>>>       }
>>>
>>>       case PERF_EVENT_IOC_SET_FILTER:
>>> +   case PERF_EVENT_IOC_SET_FILTER_32:
>>>           return perf_event_set_filter(event, (void __user *)arg);
>>>
>>>       default:
>>>
>>
>> Oh, PERF_EVENT_IOC_ID has the same problem:
>>
>> #define PERF_EVENT_IOC_ID               _IOR('$', 7, __u64 *)
>
> The right way is to add a compat_perf_ioctl()

Sure, looked into that way as well. But SET_FILTER and IOC_ID will still 
compile to the same values for a 64-bit kernel.

David

Re: PERF_EVENT_IOC_SET_FILTER has different values based on bitness

[Andi Kleen]

David Ahern <dsahern@gmail.com> writes:

> On 10/7/14, 6:50 PM, Andi Kleen wrote:
>> David Ahern <dsahern@gmail.com> writes:
>>>>
>>>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>>>> index 963bf139e2b2..c805132ac1cf 100644
>>>> --- a/kernel/events/core.c
>>>> +++ b/kernel/events/core.c
>>>> @@ -3714,6 +3714,7 @@ static long perf_ioctl(struct file *file, unsigned
>>>> int cmd, unsigned long arg)
>>>>       }
>>>>
>>>>       case PERF_EVENT_IOC_SET_FILTER:
>>>> +   case PERF_EVENT_IOC_SET_FILTER_32:
>>>>           return perf_event_set_filter(event, (void __user *)arg);
>>>>
>>>>       default:
>>>>
>>>
>>> Oh, PERF_EVENT_IOC_ID has the same problem:
>>>
>>> #define PERF_EVENT_IOC_ID               _IOR('$', 7, __u64 *)
>>
>> The right way is to add a compat_perf_ioctl()
>
> Sure, looked into that way as well. But SET_FILTER and IOC_ID will
> still compile to the same values for a 64-bit kernel.

Sure you have to add/use the new defines too.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only

Re: PERF_EVENT_IOC_SET_FILTER has different values based on bitness

[Peter Zijlstra]

On Tue, Oct 07, 2014 at 05:07:36PM -0600, David Ahern wrote:
> 
> 32-bit perf binaries are not able to set filters on 64-bit kernels.
> 
>    $ perf record -e  net:netif_receive_skb --filter 'name == "eth1"
>    Error: failed to set filter with 25 (Inappropriate ioctl for device)
> 
> The reason is that the definition of PERF_EVENT_IOC_SET_FILTER contains a
> pointer:
> 
> #define PERF_EVENT_IOC_SET_FILTER       _IOW('$', 6, char *)
> 
> the size of which of course differs for 32-bit and 64-bit. This has been
> there since the original commit (6fb2915df7f07) back in 2009.
> 

Should be fixed for a while now

---
commit b3f207855f57b9c8f43a547a801340bb5cbc59e5
Author: Pawel Moll <pawel.moll@arm.com>
Date:   Fri Jun 13 16:03:32 2014 +0100

    perf: Handle compat ioctl
    
    When running a 32-bit userspace on a 64-bit kernel (eg. i386
    application on x86_64 kernel or 32-bit arm userspace on arm64
    kernel) some of the perf ioctls must be treated with special
    care, as they have a pointer size encoded in the command.
    
    For example, PERF_EVENT_IOC_ID in 32-bit world will be encoded
    as 0x80042407, but 64-bit kernel will expect 0x80082407. In
    result the ioctl will fail returning -ENOTTY.
    
    This patch solves the problem by adding code fixing up the
    size as compat_ioctl file operation.
    
    Reported-by: Drew Richardson <drew.richardson@arm.com>
    Signed-off-by: Pawel Moll <pawel.moll@arm.com>
    Signed-off-by: Peter Zijlstra <peterz@infradead.org>
    Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
    Cc: Jiri Olsa <jolsa@redhat.com>
    Link: http://lkml.kernel.org/r/1402671812-9078-1-git-send-email-pawel.moll@arm.com
    Signed-off-by: Ingo Molnar <mingo@kernel.org>

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 1cf24b3e42ec..f9c1ed002dbc 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -41,6 +41,7 @@
 #include <linux/cgroup.h>
 #include <linux/module.h>
 #include <linux/mman.h>
+#include <linux/compat.h>
 
 #include "internal.h"
 
@@ -3717,6 +3718,26 @@ static long perf_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	return 0;
 }
 
+#ifdef CONFIG_COMPAT
+static long perf_compat_ioctl(struct file *file, unsigned int cmd,
+				unsigned long arg)
+{
+	switch (_IOC_NR(cmd)) {
+	case _IOC_NR(PERF_EVENT_IOC_SET_FILTER):
+	case _IOC_NR(PERF_EVENT_IOC_ID):
+		/* Fix up pointer size (usually 4 -> 8 in 32-on-64-bit case */
+		if (_IOC_SIZE(cmd) == sizeof(compat_uptr_t)) {
+			cmd &= ~IOCSIZE_MASK;
+			cmd |= sizeof(void *) << IOCSIZE_SHIFT;
+		}
+		break;
+	}
+	return perf_ioctl(file, cmd, arg);
+}
+#else
+# define perf_compat_ioctl NULL
+#endif
+
 int perf_event_task_enable(void)
 {
 	struct perf_event *event;
@@ -4222,7 +4243,7 @@ static const struct file_operations perf_fops = {
 	.read			= perf_read,
 	.poll			= perf_poll,
 	.unlocked_ioctl		= perf_ioctl,
-	.compat_ioctl		= perf_ioctl,
+	.compat_ioctl		= perf_compat_ioctl,
 	.mmap			= perf_mmap,
 	.fasync			= perf_fasync,
 };

Re: PERF_EVENT_IOC_SET_FILTER has different values based on bitness

[David Ahern]

On 10/8/14, 1:23 AM, Peter Zijlstra wrote:
> Should be fixed for a while now
>
> ---
> commit b3f207855f57b9c8f43a547a801340bb5cbc59e5
> Author: Pawel Moll <pawel.moll@arm.com>
> Date:   Fri Jun 13 16:03:32 2014 +0100
>
>      perf: Handle compat ioctl
>

that works. Can we get this queued to the stable releases (at least as 
far back as v3.4)?

Thanks,
David

Re: PERF_EVENT_IOC_SET_FILTER has different values based on bitness

[Arnaldo Carvalho de Melo]

Em Wed, Oct 08, 2014 at 09:17:38AM -0600, David Ahern escreveu:
> On 10/8/14, 1:23 AM, Peter Zijlstra wrote:
> >Should be fixed for a while now
> >
> >---
> >commit b3f207855f57b9c8f43a547a801340bb5cbc59e5
> >Author: Pawel Moll <pawel.moll@arm.com>
> >Date:   Fri Jun 13 16:03:32 2014 +0100
> >
> >     perf: Handle compat ioctl
> >
> 
> that works. Can we get this queued to the stable releases (at least as far
> back as v3.4)?

I wonder how that works best, being on the receiving side of such requests
from time to time.

I guess that to scale, that would be better done by:

1. Reporter tests if the patch applies (and works) on the desired
targets.

2. Reporter sends the request, with the above test results, to
stable@kernel.org, following whatever conventions are to get the
attention of the stable release maintainers.

- Arnaldo

Re: PERF_EVENT_IOC_SET_FILTER has different values based on bitness

[David Ahern]

On 10/8/14, 9:22 AM, Arnaldo Carvalho de Melo wrote:
> I wonder how that works best, being on the receiving side of such requests
> from time to time.
>
> I guess that to scale, that would be better done by:
>
> 1. Reporter tests if the patch applies (and works) on the desired
> targets.
>
> 2. Reporter sends the request, with the above test results, to
> stable@kernel.org, following whatever conventions are to get the
> attention of the stable release maintainers.

My understanding is that subsystem maintainers do the stable requests.

A patch adjustment is needed for v3.4 and v3.10. I am willing to do the 
necessary patch mods for 3.4, 3.10, and 3.14.

David

Re: PERF_EVENT_IOC_SET_FILTER has different values based on bitness

[Ingo Molnar]

* David Ahern <dsahern@gmail.com> wrote:

> On 10/8/14, 9:22 AM, Arnaldo Carvalho de Melo wrote:
> >I wonder how that works best, being on the receiving side of such requests
> >from time to time.
> >
> >I guess that to scale, that would be better done by:
> >
> >1. Reporter tests if the patch applies (and works) on the desired
> >targets.
> >
> >2. Reporter sends the request, with the above test results, to
> >stable@kernel.org, following whatever conventions are to get the
> >attention of the stable release maintainers.
> 
> My understanding is that subsystem maintainers do the stable 
> requests.

At least for perf bits, the usual workflow is that pretty much 
anyone can make requests for backports that we maintainers 
missed, and subsystem maintainers look over them and object if 
they don't like the suggestion. (Obviously you need to Cc: 
maintainers.)

In other words, feel free to forward (tested!) backport commit 
IDs to -stable, with maintainers Cc:-ed, and feel free to provide 
conflict resolution as well, in cases where they don't apply 
cleanly.

> A patch adjustment is needed for v3.4 and v3.10. I am willing 
> to do the necessary patch mods for 3.4, 3.10, and 3.14.

That would be useful.

Thanks,

	Ingo