linux-kernel - Re: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on the same function

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]

Date:   Tue, 19 Jul 2022 19:04:20 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Song Liu <songliubraving@...com>
Cc:     Song Liu <song@...nel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "live-patching@...r.kernel.org" <live-patching@...r.kernel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        Kernel Team <Kernel-team@...com>,
        "jolsa@...nel.org" <jolsa@...nel.org>
Subject: Re: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops
 on the same function

On Tue, 19 Jul 2022 22:57:52 +0000
Song Liu <songliubraving@...com> wrote:

> >> +	hash = ops->func_hash->filter_hash;
> >> +	size = 1 << hash->size_bits;
> >> +	for (i = 0; i < size; i++) {
> >> +		hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
> >> +			unsigned long ip = entry->ip;
> >> +			bool found_op = false;
> >> +
> >> +			mutex_lock(&ftrace_lock);
> >> +			do_for_each_ftrace_op(op, ftrace_ops_list) {
> >> +				if (!(op->flags & FTRACE_OPS_FL_DIRECT))
> >> +					continue;
> >> +				if (ops_references_ip(op, ip)) {
> >> +					found_op = true;
> >> +					break;  
> > 
> > I think you want a goto here. The macros "do_for_each_ftrace_op() { .. }
> > while_for_each_ftrace_op()" is a double loop. The break just moves to the
> > next set of pages and does not break out of the outer loop.  
> 
> Hmmm... really? I didn't see it ...
> 
> 
> #define do_for_each_ftrace_op(op, list)                 \
>         op = rcu_dereference_raw_check(list);                   \
>         do
> 
> #define while_for_each_ftrace_op(op)                            \
>         while (likely(op = rcu_dereference_raw_check((op)->next)) &&    \
>                unlikely((op) != &ftrace_list_end))
> 
> Did I miss something...?

Bah, you're right. I was confusing it with do_for_each_ftrace_rec(), which
*is* a double loop.

Never mind ;-)

> 
> > 
> > 					goto out_loop;
> >   
> >> +				}
> >> +			} while_for_each_ftrace_op(op);  
> 
> [...]
> 
> >   
> >> 	mutex_lock(&ftrace_lock);
> >> 
> >> 	ret = ftrace_startup(ops, 0);
> >> 
> >> 	mutex_unlock(&ftrace_lock);
> >> 
> >> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> >> +	if (direct_mutex_locked)
> >> +		mutex_unlock(&direct_mutex);
> >> +#endif  
> > 
> > Change this to:
> > 
> > out_unlock:
> > 	mutex_unlock(&direct_mutex);
> >   
> 
> We still need #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS, as 
> direct_mutex is not defined without that config. 

Ah, right. I just meant to get rid of the if statement.

To keep the code clean, perhaps we should have:

#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
# define lock_direct_mutex()	mutex_lock(&direct_mutex)
# define unlock_direct_mutex()	mutex_unlock(&direct_mutex)
#else
# define lock_direct_mutex()	do { } while (0)
# define unlock_direct_mutex()	do { } while (0)
#endif

And use that here.

-- Steve