linux-kernel - Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace

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, 15 Oct 2019 09:45:40 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Miroslav Benes <mbenes@...e.cz>, jikos@...nel.org,
        joe.lawrence@...hat.com, jpoimboe@...hat.com, mingo@...hat.com,
        linux-kernel@...r.kernel.org, live-patching@...r.kernel.org
Subject: Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag

On Mon 2019-10-14 11:17:19, Steven Rostedt wrote:
> On Mon, 14 Oct 2019 12:59:23 +0200
> Miroslav Benes <mbenes@...e.cz> wrote:
> 
> >  int
> >  ftrace_enable_sysctl(struct ctl_table *table, int write,
> >  		     void __user *buffer, size_t *lenp,
> > @@ -6740,8 +6754,6 @@ ftrace_enable_sysctl(struct ctl_table *table, int write,
> >  	if (ret || !write || (last_ftrace_enabled == !!ftrace_enabled))
> >  		goto out;
> >  
> > -	last_ftrace_enabled = !!ftrace_enabled;
> > -
> >  	if (ftrace_enabled) {
> >  
> >  		/* we are starting ftrace again */
> > @@ -6752,12 +6764,19 @@ ftrace_enable_sysctl(struct ctl_table *table, int write,
> >  		ftrace_startup_sysctl();
> >  
> >  	} else {
> > +		if (is_permanent_ops_registered()) {
> > +			ftrace_enabled = last_ftrace_enabled;
> 
> Although this is not incorrect, but may be somewhat confusing.
> 
> At this location, last_ftrace_enabled is always true.
> 
> I'm thinking this would be better to simply set it to false here.

IMHO, we want to set ftrace_enabled = true here.

It was set to "false" by writing to the sysfs file. But the change
gets rejected. ftrace will stay enabled. So, we should set
the value back to "true".


> > +			ret = -EBUSY;
> > +			goto out;
> > +		}
> > +
> >  		/* stopping ftrace calls (just send to ftrace_stub) */
> >  		ftrace_trace_function = ftrace_stub;
> >  
> >  		ftrace_shutdown_sysctl();
> >  	}
> >  
> > +	last_ftrace_enabled = !!ftrace_enabled;
> >   out:
> 
> And move the assignment of last_ftrace_enabled to after the "out:"
> label.

This change might make sense anyway. But it is not strictly necessary
from my POV.

Best Regards,
Petr