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
