Received-SPF: pass (google.com: domain of linux-kernel+bounces-171993-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: Stephen Brennan <stephen.s.brennan@oracle.com>
To: Christophe Leroy <christophe.leroy@csgroup.eu>,
        Steven Rostedt
 <rostedt@goodmis.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Mark
 Rutland <mark.rutland@arm.com>
Cc: Guo Ren <guoren@kernel.org>, Huacai Chen <chenhuacai@kernel.org>,
        WANG
 Xuerui <kernel@xen0n.name>,
        "James E.J. Bottomley"
 <James.Bottomley@HansenPartnership.com>,
        Helge Deller <deller@gmx.de>, Michael Ellerman <mpe@ellerman.id.au>,
        Nicholas Piggin
 <npiggin@gmail.com>,
        "Aneesh Kumar K.V" <aneesh.kumar@kernel.org>,
        "Naveen
 N. Rao" <naveen.n.rao@linux.ibm.com>,
        Paul Walmsley
 <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou
 <aou@eecs.berkeley.edu>,
        Heiko
 Carstens <hca@linux.ibm.com>, Vasily Gorbik <gor@linux.ibm.com>,
        Alexander
 Gordeev <agordeev@linux.ibm.com>,
        Christian Borntraeger
 <borntraeger@linux.ibm.com>,
        Sven Schnelle <svens@linux.ibm.com>,
        Thomas
 Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
        Borislav
 Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "x86@kernel.org" <x86@kernel.org>, "H. Peter Anvin" <hpa@zytor.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-trace-kernel@vger.kernel.org" <linux-trace-kernel@vger.kernel.org>,
        "linux-csky@vger.kernel.org" <linux-csky@vger.kernel.org>,
        "loongarch@lists.linux.dev" <loongarch@lists.linux.dev>,
        "linux-parisc@vger.kernel.org" <linux-parisc@vger.kernel.org>,
        "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
        "linux-riscv@lists.infradead.org" <linux-riscv@lists.infradead.org>,
        "linux-s390@vger.kernel.org" <linux-s390@vger.kernel.org>
Subject: Re: [PATCH v3] kprobe/ftrace: bail out if ftrace was killed
In-Reply-To: <fd8283ac-232e-49e8-a5be-60e54d87b9eb@csgroup.eu>
References: <20240501162956.229427-1-stephen.s.brennan@oracle.com>
 <fd8283ac-232e-49e8-a5be-60e54d87b9eb@csgroup.eu>
Date: Tue, 07 May 2024 11:36:22 -0700
Message-ID: <87ttj9h3kp.fsf@oracle.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?Nzl5eWJiRG16Q1lVRy9yZmt3bE5KQUNNUFBWTXkxVlFBTFM5V1Rrc0hvWDBN?=
 =?utf-8?B?WVlNbC9tSmZBMjQ2czZ0Z2JQWFFVbmgrWFhDTmM0TWhYOEVLbjBYME50Vmwv?=
 =?utf-8?B?TXFHVXJiMzZmWHVJU3VDK2VLcXZWWUQwSUN0bHhRTXo2VE5DTnVPbytMdWdL?=
 =?utf-8?B?cEJnSGVCb1l0K1pBdDlBb0dLSVlqOTcraUd3cytCR1BTL3ZOTEZqSkYyaUFa?=
 =?utf-8?B?a2QwZnpxYnFuZVBKU2FJUy9ZajQrNnExYXVGVlFpNHRqWEFKSUtEQm44NHdm?=
 =?utf-8?B?b3J5endKdDZXejFLcGN5eWlxTUlWYjBiOGYvNnF0Q01BeWUwM24vaDUzN1pZ?=
 =?utf-8?B?anhKcG9YbUtLLzVvekdxbzlYY0ltZWFZNHZ2VEs0ZlRRY00zUzdpZklnbnk0?=
 =?utf-8?B?bTF2a2owSkZJK1V2UjBDcXNneHVIWDZST0NEM1JDSWFFU2RnNXBUTUZnTjh0?=
 =?utf-8?B?dDJNMkRhZHVjam9vWC9uMlpiSWNybC8xK1lTU3NjbTBOQWVwNkxqNW45UWhY?=
 =?utf-8?B?dVNZYXdvT1g5blVTanhEUElGRHhJaEVTdWZmTFMvbFFZY0ZRcDhhdEZ3eG5H?=
 =?utf-8?B?dzNnOEZ4eTR1cnVEbTgvOVZwTDRkWjQyem9CMlY4ODgyNVNuY3dqYmxXbGlL?=
 =?utf-8?B?YWJhRXl6ajUzOHhSUkNqT0lYZHRNbEhSY1BsdlRONDBRT08wV1NjRzVaV2pW?=
 =?utf-8?B?b0d5NzBYcjlBMGpqc05udU4zbG1aTktRelhkVm9lbFpadmI1QktXWUxOTk9K?=
 =?utf-8?B?aXcvOXh2STVwT0ZQNllGZFBCSVRjQWdPMVlmR3Jka0dtbE9pN0JQTlZwTFdP?=
 =?utf-8?B?Z1lkeWFMblVKYXBBY0FITjBBSnVBSmVOWVZNWERmMHcwRjVzOFYyUVd0ZHh6?=
 =?utf-8?B?bmZkcXc1MytCYk9HRksrdTFmNDM4Yk1qL21pajJuWnVxc3prZExaVW0xVjJ6?=
 =?utf-8?B?ei9BeDdHZXZIa0c2MXBQK0FWV1l2UFZndjJTSklRNHNub1BGT3JLeDVscEw4?=
 =?utf-8?B?Z3FWQ3g3MFI4S1hZcmEvblZVdmNyS2NlOE0rUDNHMEdiR21qbFhqbmtyYU1s?=
 =?utf-8?B?bjZNNWtNbEtKNGJTYyt4cHJneEZVQSs2MFFzdjFhVzc1d2I4V1ZERGdpNmpY?=
 =?utf-8?B?SDlLUDZQNUVWdzFOSkJETTloTlpybWh3R2JFN3UvK0ZpbmR0V3ZNdlpZbzkz?=
 =?utf-8?B?dzZJZ3pIb01ja0pxNk45S1VmWEt1R2VRZ1pLLzlLQmRHR09yZWFsM0VYcWhK?=
 =?utf-8?B?NzdRZzNYTndLVWZoWkRqZVYxTHZPTVpqWm5DSk84YjJFTFBVUzVPUU9WK0l3?=
 =?utf-8?B?RkpQY2tLUTA5RHB6cVp3c25BTHlza2d4ZkExUUxwNEFIWkdjRGhVVVJhZlkw?=
 =?utf-8?B?YW1tZ1U0N2tKczZwUWRFOHhOS0JpNE1OTUd0Mmp4N0liNTlQZHZvazc1UVl6?=
 =?utf-8?B?OVJObVF0NTUyeC8wMmxPb0hNclRZYm9ENG1QUkN4enRwNVZvZVFWRnBid0tl?=
 =?utf-8?B?b1EvOFF4MnJLUWs2bVhZQ3RKMmxiODl1cFZzcGNLR0d5RDNwdUdxcnNHekp1?=
 =?utf-8?B?WExPaHdYQVB0QVY4RTNxMkpsbjdVT1FnZTFkKzRWSGtObW0rajF2YnMxZVEx?=
 =?utf-8?B?SnZlKzhEclFLOGRWTzVHM0xJL0dvTXBOS3VOTWtkZk42QWV1N1hCUHEvbXJW?=
 =?utf-8?B?cWJhTDdVdFUxenNxRUptRGtxRkdVSExRQWRZcy81cUJmVTUrNk5lUSs5NDZu?=
 =?utf-8?B?Q0hGWUw5Z0ZKMWx1c1AxVTlXVzFQK2RtMUI3UU9idHI3ZTBnSHhKVTh6ejZh?=
 =?utf-8?B?dHNXZkdpNnlERUs4eU9QTmpFL0R1eUlSSFR3MFRlaXRQVnQwVVl4K1JlUGZI?=
 =?utf-8?B?aGtLdXQ1NlZ0eGFEc2FwRk8xQkFnb3FPQkVOWUVvTlZzTVk3MDdPendsOFRw?=
 =?utf-8?B?bnNKTmpJd1Bwb1JERkNTUkRFR3BuZXhrVTFFVmw3dUhhdEhBL3Vxb0VIVGxC?=
 =?utf-8?B?TjczS2U5S3IwTXp0R1BzTTBSNkh1R1hicTRRYndsKy8zV0MvZVM4MXZPejky?=
 =?utf-8?B?VVIzc25RWExxZmdtVXJEVTNEWGhESEVmTXBxREFDUW1pbzhMVnc0QkhyRzF6?=
 =?utf-8?B?QVVaMmhObHdtZHhrWXBGd1U3UkJPNFBYZWpkRDRFWmJrcm5WcVNXRnNic0Nv?=
 =?utf-8?Q?haBgcaU/iOiriI6vHqJmqfg=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	GyWSZpflPjHJQXVh/b1/6ZH2xUHXQdYVOZw+gFcBkfqmv8XH/mTImkhSx7NOM3v81Q6rssqmZB7qct3ggSGKA/Eda8mL/p5QjdL8ocsxlsgKvXDcr6QCMOUUysL8+BwfO7/1QuHV5GvryYaH55uoA2PNU9Q3HZy33isLuMkuiYTcFvqSzITQs2C9vKgF/yR6HvLcmSMguWL8iP8/gZXs+vOoI4jP5y9KEW3qGFwBCbUyu2K7oLC/0zox2uVmTh4vvSFFX8m7/39pnWUoNlF28offR8hrxFIPBCktYQXyLlz0MD79V4d8bPj/2APlNjM6mTvKNRAg6wsT154sNm46FZ0s0mFDm5nS/naOzZ2KtlqpW0dekmCV+Nl1TUoFeXifEaDaMztA2RbqNWGXp/2CyJXPkcOdtH8VbVJLDBt7jjgjt3BQhucMK7rD7zQr72ewbpy3XawpxsBLkQukamC+buo3RtZ0e1Nkvk+k6AbEhaz4l4634maaaasbJBFLkRJSl52BH8voN6PRZgpQPkiPbt+YMBaRA/PjO55kzki0QVEJL9XfWa7VC+Vx/cG2Ikt9587A/qvOR75XQx3gBlftOmizLHUs2Kqp/2ReQKDGOYI=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ded9d75e-bc25-49eb-49f4-08dc6ec4996d
X-MS-Exchange-CrossTenant-AuthSource: PH8PR10MB6597.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 May 2024 18:36:24.9008
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: WwOZnOdTcj9ULTuqz/0avjVKx9btM5qxYc0NMnjYQFMlaisO/0u5IjF03NoaoJjK95a7QmQlppYstMra0ggegeG22juA3e7wn+gl49mVWfI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN0PR10MB5175
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.650,FMLib:17.11.176.26
 definitions=2024-05-07_11,2024-05-06_02,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 phishscore=0
 suspectscore=0 spamscore=0 bulkscore=0 mlxlogscore=999 adultscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2405010000 definitions=main-2405070130
X-Proofpoint-ORIG-GUID: wo5v_Q4_FFO82DYzsVnZnT63OzaEeOFg
X-Proofpoint-GUID: wo5v_Q4_FFO82DYzsVnZnT63OzaEeOFg

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 01/05/2024 =C3=A0 18:29, Stephen Brennan a =C3=A9crit=C2=A0:
>> If an error happens in ftrace, ftrace_kill() will prevent disarming
>> kprobes. Eventually, the ftrace_ops associated with the kprobes will be
>> freed, yet the kprobes will still be active, and when triggered, they
>> will use the freed memory, likely resulting in a page fault and panic.
>>=20
>> This behavior can be reproduced quite easily, by creating a kprobe and
>> then triggering a ftrace_kill(). For simplicity, we can simulate an
>> ftrace error with a kernel module like [1]:
>>=20
>> [1]: https://github.com/brenns10/kernel_stuff/tree/master/ftrace_killer
>>=20
>>    sudo perf probe --add commit_creds
>>    sudo perf trace -e probe:commit_creds
>>    # In another terminal
>>    make
>>    sudo insmod ftrace_killer.ko  # calls ftrace_kill(), simulating bug
>>    # Back to perf terminal
>>    # ctrl-c
>>    sudo perf probe --del commit_creds
>>=20
>> After a short period, a page fault and panic would occur as the kprobe
>> continues to execute and uses the freed ftrace_ops. While ftrace_kill()
>> is supposed to be used only in extreme circumstances, it is invoked in
>> FTRACE_WARN_ON() and so there are many places where an unexpected bug
>> could be triggered, yet the system may continue operating, possibly
>> without the administrator noticing. If ftrace_kill() does not panic the
>> system, then we should do everything we can to continue operating,
>> rather than leave a ticking time bomb.
>>=20
>> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
>> ---
>> Changes in v3:
>>    Don't expose ftrace_is_dead(). Create a "kprobe_ftrace_disabled"
>>    variable and check it directly in the kprobe handlers.
>
> Isn't it safer to provide a fonction rather than a direct access to a=20
> variable ?

Is the concern that other code could modify this variable? If so, then I
suppose the function call is safer. But the variable is not exported and
I think built-in code can be trusted not to muck with it. Maybe I'm
missing your point about safety though?

> By the way, wouldn't it be more performant to use a static branch (jump=20
> label) ?

I agree with Steven's concern that text modification would unfortunately
not be a good way to handle an error in text modification. Especially, I
believe there could be deadlock risks, as static key enablement requires
taking the text_mutex and the jump_label_mutex. I'd be concerned that
the text_mutex could already be held in some situations where
ftrace_kill() is called. But I'm not certain about that.

Thanks for taking a look!
Stephen