Re: [PATCH V8 01/16] rv: Add Runtime Verification (RV) interface

[Daniel Bristot de Oliveira]

On 7/28/22 19:36, Tao Zhou wrote:
> On Wed, Jul 27, 2022 at 07:11:29PM +0200, Daniel Bristot de Oliveira wrote:
> 
>> +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];
> 
> If I am not wrong, but "joke" from myself is very possible.
> 
> char buff[MAX_RV_MONITOR_NAME_SIZE + 1];
> 
> +1 is for one '\0'. The above have '\0\0'. One '\0' is enough.

!

>> +	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)
> 
> Use `count > MAX_RV_MONITOR_NAME_SIZE` check the up bound.
> 
>> +		return -EINVAL;
>> +
>> +	memset(buff, 0, sizeof(buff));
>> +
>> +	retval = simple_write_to_buffer(buff, sizeof(buff) - 1, ppos, user_buf, count);
> 
> simple_write_to_buffer(buff, sizeof(buff), ppos, user_buf, count)
> 
>> +	if (retval < 0)
>> +		return -EFAULT;
>> +
>> +	ptr = strim(buff);
> 
> I see isspace() that the mask `_S` is for space/lf/tab, but I do
> not know if the lf stands for being able to strim the '\n'. If so
> there is no problem here. if use buffer is "wip\n\n", we should
> treat it the same as "wip", no?

no.

> 
>> +/*
>> + * Monitoring on global switcher!
>> + */
>> +static bool __read_mostly monitoring_on;
>> +
>> +/**
>> + * rv_monitoring_on - checks if monitoring is on
>> + *
>> + * Returns 1 if on, 0 otherwise.
>> + */
>> +bool rv_monitoring_on(void)
>> +{
>> +	/* Ensures that concurrent monitors read consistent monitoring_on */
>> +	smp_rmb();
> 
> Here invalidate message will be processed and send the read message
> and get updated monitoring_on from another cpu. I feel confused
> because there is half part of the memory barrier pair. But this half
> way from my mind in this case has effect. This is the first time that
> I know it can be synced this way. Let me guess this way.
> 
>> +	return READ_ONCE(monitoring_on);
>> +}
> 
> I checked the load of monitoring_on, there are three cases:
> file read     file write(call load self)     event handler check
> Store of monitoring_on: one in init rv, another is file write after
> call load self.
> The file is created before the turn_monitoring_on() called in 
> rv_init_interface(). So there may be existing the store race
> at the init part. Just after the monitoring_on file created,
> and other cpus do monitoring_on flips operations and at the
> same time the init code do turn_monitor_on(). Or the enabled
> file be writen to enable/disable monitors happening before
> monitoring_on is set in init rv. That means the event handler
> can be start before the monitoring_on is turned on in init rv.
> The turn_monitoring_on() in rv_init_interface() is not a switcher
> because it may has been beated by file flips operations before.

there will be no monitors loaded at this point during boot time.

>> +
>> +/*
>> + * monitoring_on general switcher.
>> + */
>> +static ssize_t monitoring_on_read_data(struct file *filp, char __user *user_buf,
>> +				       size_t count, loff_t *ppos)
>> +{
>> +	const char *buff;
>> +
>> +	buff = rv_monitoring_on() ? "1\n" : "0\n";
> 
> I hope this will not be inlined..

Even if I add a lock, the value can change after the lock is unlocked before
returning to user-space...

> 
>> +
>> +	return simple_read_from_buffer(user_buf, count, ppos, buff, strlen(buff) + 1);
>> +}
>> +static void destroy_monitor_dir(struct rv_monitor_def *mdef)
>> +{
>> +	reactor_cleanup_monitor(mdef);
> 
> reactor_cleanup_monitor() appear in this patch but not defined.

I will have to send a v9 only fixing this because it breaks bisect.

It was caused by a last minute change... (boooh, Daniel!)

>> +	rv_remove(mdef->root_d);
>> +}
>> +struct dentry *get_monitors_root(void);
>> +int init_rv_monitors(struct dentry *root_dir);
> 
> init_rv_monitors() definition do not appear in this patch. Thanks,

Thanks!
-- Daniel