Date:   Sat, 30 Jul 2022 20:07:07 +0200
From:   Daniel Bristot de Oliveira <bristot@...nel.org>
To:     Tao Zhou <tao.zhou@...ux.dev>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Wim Van Sebroeck <wim@...ux-watchdog.org>,
        Guenter Roeck <linux@...ck-us.net>,
        Jonathan Corbet <corbet@....net>,
        Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Will Deacon <will@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Marco Elver <elver@...gle.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Shuah Khan <skhan@...uxfoundation.org>,
        Gabriele Paoloni <gpaoloni@...hat.com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Clark Williams <williams@...hat.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-trace-devel@...r.kernel.org
Subject: Re: [PATCH V9 01/16] rv: Add Runtime Verification (RV) interface

On 7/30/22 16:08, Tao Zhou wrote:
> On Fri, Jul 29, 2022 at 11:38:40AM +0200, Daniel Bristot de Oliveira wrote:
> 
>> +static int __rv_disable_monitor(struct rv_monitor_def *mdef, bool sync)
>> +{
>> +	lockdep_assert_held(&rv_interface_lock);
>> +
>> +	if (mdef->monitor->enabled) {
>> +		mdef->monitor->enabled = 0;
>> +		mdef->monitor->disable();
> 
> If call disable(), the @enabled is set 0 there.

Which is correct.

> 
>> +
>> +		/*
>> +		 * Wait for the execution of all events to finish.
>> +		 * Otherwise, the data used by the monitor could
>> +		 * be inconsistent. i.e., if the monitor is re-enabled.
>> +		 */
>> +		if (sync)
>> +			tracepoint_synchronize_unregister();
>> +		return 1;
> 
> Return 0 indicate the actually disabling and successed.

No, 1 indicates that *disable was called, 0 did not call disable function.

>> +	}
>> +	return 0;
> 
> If disable a diabled monitor, return error(negative).

This is a "static __function", which alerts for internal aspects.

It has a specific purpose of counting if the disable function
was actually called.

Disabling a disabled monitor is not a problem.

So all your argumentation based on this is not correct, and it is breaking
other parts of the code... see where it is called.

>> +}
>> +
>> +/**
>> + * rv_disable_monitor - disable a given runtime monitor
>> + *
>> + * Returns 0 on success.
>> + */
>> +int rv_disable_monitor(struct rv_monitor_def *mdef)
>> +{
>> +	__rv_disable_monitor(mdef, true);
>> +	return 0;
> 
> Always return 0 here, whatever the return value of __rv_disable_monitor().
> And this enforce me to look more here, see below.

This is not a problem. Actually, disable functions often return void.
I am keeping an int just in case.

>> +}
> 
>> +static ssize_t enabled_monitors_write(struct file *filp, const char __user *user_buf,
>> +				      size_t count, loff_t *ppos)
>> +{
>> +	char buff[MAX_RV_MONITOR_NAME_SIZE + 2];
>> +	struct rv_monitor_def *mdef;
>> +	int retval = -EINVAL;
>> +	bool enable = true;
>> +	char *ptr = buff;
>> +	int len;
>> +
>> +	if (count < 1 || count > MAX_RV_MONITOR_NAME_SIZE + 1)
>> +		return -EINVAL;
>> +
>> +	memset(buff, 0, sizeof(buff));
>> +
>> +	retval = simple_write_to_buffer(buff, sizeof(buff) - 1, ppos, user_buf, count);
>> +	if (retval < 0)
>> +		return -EFAULT;
>> +
>> +	ptr = strim(buff);
>> +
>> +	if (ptr[0] == '!') {
>> +		enable = false;
>> +		ptr++;
>> +	}
>> +
>> +	len = strlen(ptr);
>> +	if (!len)
>> +		return count;
>> +
>> +	mutex_lock(&rv_interface_lock);
>> +
>> +	retval = -EINVAL;
>> +
>> +	list_for_each_entry(mdef, &rv_monitors_list, list) {
>> +		if (strcmp(ptr, mdef->monitor->name) != 0)
>> +			continue;
>> +
>> +		/*
>> +		 * Monitor found!
>> +		 */
>> +		if (enable)
>> +			retval = rv_enable_monitor(mdef);
>> +		else
>> +			retval = rv_disable_monitor(mdef);
> 
> About the retval here. If count == 1 and retval == 0, then
> `retval = count` --> retval == 1. This retval will be returned to 
> user space and dedicate that how many character read and success
> If retval is 1(it is not possiable, the return value of
> da_monitor_init_*() called in enable callback in rv_enable_monitor()
> will be 0, so that return value check is not needed, or any other functions
> called in enable callback need to check the return value then,...

All things above are misled by the first interpretation but,,,

so I checked
> the WARN_ONCE() called in macro rv_attach_trace_probe() which is called in
> enable callback,if the WARN_ONCE is called, it means that something go wrong.

The way that rv_attach_trace_probe() is attaching a probe is not different from the way
other *in kernel* tracing does.

> We need to check the return value of WARN_ONCE() in enable callback), the
> return value will be returned to user space but actually the error(warn) happened.
> User space do not know. They treat the two kind of return value 1 the same
> but one is the write count value successed and another is the write error
> value returned.
> In enable callback, check rv_attach_trace_probe():
> 
> static int enable_wip(void)
> {
>       int retval = 1;
>  	  
>       /* 
>        * Delete the check of return value of da_monitor_init_wip()
>        * because it is always 0
>        */
>       da_monitor_init_wip();
> 
>       retval &= rv_attach_trace_probe("wip", preempt_enable, handle_preempt_enable);
>       retval &= rv_attach_trace_probe("wip", sched_waking, handle_sched_waking);
>       retval &= rv_attach_trace_probe("wip", preempt_disable, handle_preempt_disable);

No, that is not the most robust way to do this. A better way is to do it like in the
early versions of this patch set, where it was searching for the existence of the tracepoint
from the module perspective, taking notes of the ones that were enabled, and then actually disabling
all events that were enabled before the failure.

> 
>       /* 
>        * If the retval is not 0, it mean at least one rv_attach_trace_probe()
>        * is WARN_ONCE(). I am not sure that if the first WARN_ONCE() happened,
>        * then return directly or at here after all rv_attach_trace_probe() is
>        * called and check the retval is 0 or 1.
>        */
>       if (retval)
>              return -1;

and here the system state is even worse than WARNING and doing nothing: the monitor is
disabled, but the tracepoints that were registered are still hooked to the system...
and you cannot unhook them because the monitor is not enabled.

You still can unhook in current implementation.

So, for the in-kernel version, the current method is equivalent to the
the way we do on other tracers, and the monitors only compile if the
tracepoints exist, the callback has the correct signature and WARNs
in case of problems in the tracepoint.

There will be a more robust way to do this, and it will be included in the
"add module support" patch set. But is its better to add it in a patch
set because we can analyze change by change instead of adding on top
of this initial implementation - which is quite large already.

>       return retval;
> }
> 
>> +
>> +		if (!retval)
>> +			retval = count;
>> +
>> +		break;
>> +	}
> 
>> +/**
>> + * rv_register_monitor - register a rv monitor.
>> + * @monitor:    The rv_monitor to be registered.
>> + *
>> + * Returns 0 if successful, error otherwise.
>> + */
>> +int rv_register_monitor(struct rv_monitor *monitor)
>> +{
>> +	struct rv_monitor_def *r;
>> +	int retval = 0;
>> +
>> +	if (strlen(monitor->name) >= MAX_RV_MONITOR_NAME_SIZE) {
> 
> s/>=/>/ no? The same check happened in patch 2. Thanks,

Yep, this can be improved. But it is not a BUG, as it is allowing monitor
 with (MAX_RV_MONITOR_NAME_SIZE - 1) size, which is safe.

Given that neither 'wip' or 'wwnr' are >= MAX_RV_MONITOR_NAME_SIZE, this
problem is not happening - and no other monitor can hit it because modules
are not yet supported.

I took note and will patch it.

>> +		pr_info("Monitor %s has a name longer than %d\n", monitor->name,
>> +			MAX_RV_MONITOR_NAME_SIZE);

Thanks!
-- Daniel

Powered by blists - more mailing lists