linux-kernel - Re: [PATCH 6/7] uprobes: Allow the use of uprobe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <83cf1b26-3a30-47fd-93e9-84903193bf07@arm.com>
Date: Wed, 19 Mar 2025 11:40:35 -0500
From: Jeremy Linton <jeremy.linton@....com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: linux-trace-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
 mhiramat@...nel.org, peterz@...radead.org, acme@...nel.org,
 namhyung@...nel.org, mark.rutland@....com,
 alexander.shishkin@...ux.intel.com, jolsa@...nel.org, irogers@...gle.com,
 adrian.hunter@...el.com, kan.liang@...ux.intel.com,
 thiago.bauermann@...aro.org, broonie@...nel.org, yury.khrustalev@....com,
 kristina.martsenko@....com, liaochang1@...wei.com, catalin.marinas@....com,
 will@...nel.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/7] uprobes: Allow the use of uprobe_warn() in arch code

Hi,

On 3/19/25 9:51 AM, Oleg Nesterov wrote:
> On 03/18, Jeremy Linton wrote:
>>
>> --- a/include/linux/uprobes.h
>> +++ b/include/linux/uprobes.h
>> @@ -185,6 +185,7 @@ struct uprobes_state {
>>   };
>>   
>>   extern void __init uprobes_init(void);
>> +extern void uprobe_warn(struct task_struct *t, const char *msg);
>>   extern int set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
>>   extern int set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
>>   extern bool is_swbp_insn(uprobe_opcode_t *insn);
>> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
>> index b4ca8898fe17..613c1c76f227 100644
>> --- a/kernel/events/uprobes.c
>> +++ b/kernel/events/uprobes.c
>> @@ -118,7 +118,7 @@ struct xol_area {
>>   	unsigned long 			vaddr;		/* Page(s) of instruction slots */
>>   };
>>   
>> -static void uprobe_warn(struct task_struct *t, const char *msg)
>> +void uprobe_warn(struct task_struct *t, const char *msg)
>>   {
>>   	pr_warn("uprobe: %s:%d failed to %s\n", current->comm, current->pid, msg);
>>   }
> 
> Oh, no, please don't.
> 
> uprobe_warn() is ugly and needs changes. If nothing else it doesn't even use
> its "t" argument.

Ha, I didn't look that closely at it. That is basically the same bug 1/7 
here is fixing for the gcs task function!

This is in its own patch to allow it to be easily dropped, which is what 
will happen as I'm aware of previous variations of this discussion.

While Mark R's perspective is valid, it remains worthwhile to again 
point out that the uprobes subsystem (and a few like it) is a bit of a 
mystery box. Some of these error conditions are very opaque for a user 
who isn't also sufficiently familiar with uprobes to be able to both 
find the code as well as understand or instrument it when it tosses an 
error. Discoverability is made more difficult by the extensive use of 
inline/static. End users try to work around this. For example Brendan 
Gregg's perf-tools uprobe wrapper will dump the last two lines of the 
kernel log when it hits unexpected errors.