Le 09/06/2025 à 13:00, Takashi Iwai a écrit :
On Mon, 09 Jun 2025 12:02:00 +0200,
Christophe Leroy wrote:
Le 09/06/2025 à 10:10, Takashi Iwai a écrit :
On Mon, 09 Jun 2025 10:00:38 +0200,
Christophe Leroy wrote:
With user access protection (Called SMAP on x86 or KUAP on powerpc)
each and every call to get_user() or put_user() performs heavy
operations to unlock and lock kernel access to userspace.
To avoid that, perform user accesses by blocks using
user_access_begin/user_access_end() and unsafe_get_user()/
unsafe_put_user() and alike.
As an exemple, before the patch the 9 calls to put_user() at the
end of snd_pcm_ioctl_sync_ptr_compat() imply the following set of
instructions about 9 times (access_ok - enable user - write - disable
user):
0.00 : c057f858: 3d 20 7f ff lis r9,32767
0.29 : c057f85c: 39 5e 00 14 addi r10,r30,20
0.77 : c057f860: 61 29 ff fc ori r9,r9,65532
0.32 : c057f864: 7c 0a 48 40 cmplw r10,r9
0.36 : c057f868: 41 a1 fb 58 bgt c057f3c0
<snd_pcm_ioctl+0xbb0>
0.30 : c057f86c: 3d 20 dc 00 lis r9,-9216
1.95 : c057f870: 7d 3a c3 a6 mtspr 794,r9
0.33 : c057f874: 92 8a 00 00 stw r20,0(r10)
0.27 : c057f878: 3d 20 de 00 lis r9,-8704
0.28 : c057f87c: 7d 3a c3 a6 mtspr 794,r9
...
A perf profile shows that in total the 9 put_user() represent 36% of
the time spent in snd_pcm_ioctl() and about 80 instructions.
With this patch everything is done in 13 instructions and represent
only 15% of the time spent in snd_pcm_ioctl():
0.57 : c057f5dc: 3d 20 dc 00 lis r9,-9216
0.98 : c057f5e0: 7d 3a c3 a6 mtspr 794,r9
0.16 : c057f5e4: 92 7f 00 04 stw r19,4(r31)
0.63 : c057f5e8: 93 df 00 0c stw r30,12(r31)
0.16 : c057f5ec: 93 9f 00 10 stw r28,16(r31)
4.95 : c057f5f0: 92 9f 00 14 stw r20,20(r31)
0.19 : c057f5f4: 92 5f 00 18 stw r18,24(r31)
0.49 : c057f5f8: 92 bf 00 1c stw r21,28(r31)
0.27 : c057f5fc: 93 7f 00 20 stw r27,32(r31)
5.88 : c057f600: 93 36 00 00 stw r25,0(r22)
0.11 : c057f604: 93 17 00 00 stw r24,0(r23)
0.00 : c057f608: 3d 20 de 00 lis r9,-8704
0.79 : c057f60c: 7d 3a c3 a6 mtspr 794,r9
Note that here the access_ok() in user_write_access_begin() is skipped
because the exact same verification has already been performed at the
beginning of the fonction with the call to user_read_access_begin().
A couple more can be converted as well but require
unsafe_copy_from_user() which is not defined on x86 and arm64, so
those are left aside for the time being and will be handled in a
separate patch.
Signed-off-by: Christophe Leroy <christophe.le...@csgroup.eu>
---
v2: Split out the two hunks using copy_from_user() as unsafe_copy_from_user()
is not implemented on x86 and arm64 yet.
Thanks for the patch.
The idea looks interesting, but the implementations with
unsafe_get_user() leads to very ugly goto lines, and that's too bad;
it makes the code flow much more difficult to follow.
I guess that, in most cases this patch tries to cover, we just use
another temporary variable for compat struct, copy fields locally,
then run copy_to_user() in a shot instead.
Thanks for looking.
I'll give it a try but I think going through a local intermediate will
be less performant than direct copy with unsafe_get/put_user().
I have now tried going through a temporary struct and the result is
awful, even worth than the current implementation, because
snd_pcm_ioctl_sync_ptr_compat() is not inlined anymore into
snd_pcm_ioctl(), and the call to copy_from_user() and copy_to_user()
also show up in the top 10 functions in the perf profile.
Yes, but the code readability is often more important than minor
optimizations unless it's in a hot path.
So let's focus on the identified hot path: the SNDRV_PCM_IOCTL_SYNC_PTR
iotcl.
I will send a patch focussing only on that part. I tweaked it a bit to
increase readability by nesting the failure labels closer to the
actions. Let me know whether that patch is more acceptable for you.
In parallel I will send a RFC series that reworks a bit deaper the
SNDRV_PCM_IOCTL_SYNC_PTR functions with a helper macro, maybe you will
prefer that allthough the churn is bigger.
Christophe
