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:36:42 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Miroslav Benes <mbenes@...e.cz>
Cc:     Petr Mladek <pmladek@...e.com>, 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 Tue, 15 Oct 2019 12:50:59 +0200 (CEST)
Miroslav Benes <mbenes@...e.cz> wrote:

> > > > @@ -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.

Yes, I meant true (don't know why I said false :-/ )

> > 
> > 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".  
> 
> That's correct.
> 
> I can make it explicit as proposed. I just thought that 'ftrace_enabled = 
> last_ftrace_enabled' was clear enough given Petr's explanation.
> 
> > > > +			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.  
> 
> If it stays before "out:" label, last_ftrace_enabled is set if and only if 
> it has to be set. I think it is better, but I can, of course, move it in 
> v3 if Steven prefers it.

I don't have any strong feelings here. If you want to keep it like
this, I wont argue.

-- Steve