On 21/03/2018 19:57, Jason Gunthorpe wrote:
> On Tue, Mar 20, 2018 at 07:28:19AM +0000, Mark Bloch wrote:
>> Currently access to hardware stats buffer isn't protected, this can
>> result in multiple writes and reads at the same time to the same
>> memory location. This can lead to providing an incorrect value to
>> the user. Add a mutex to protect against it.
>>
>> Fixes: b40f4757daa1 ("IB/core: Make device counter infrastructure dynamic")
>> Signed-off-by: Mark Bloch <markb@xxxxxxxxxxxx>
>> We might want to protect setting/reading lifespan as well, but it seems
>> not needed as it shouldn't be changed too often, and even if it does not by
>> multiple threads at the same time.
>>
>> v0 -> v1: Removed RFC tag and added a Fixes line.
>> drivers/infiniband/core/sysfs.c | 10 ++++++++--
>> include/rdma/ib_verbs.h | 1 +
>> 2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
>> index 8ae1308e..b769d61 100644
>> +++ b/drivers/infiniband/core/sysfs.c
>> @@ -810,10 +810,15 @@ static ssize_t show_hw_stats(struct kobject *kobj, struct attribute *attr,
>> dev = port->ibdev;
>> stats = port->hw_stats;
>> }
>> + mutex_lock(&stats->lock);
>> ret = update_hw_stats(dev, stats, hsa->port_num, hsa->index);
>> if (ret)
>> - return ret;
>> - return print_hw_stat(stats, hsa->index, buf);
>> + goto unlock;
>> + ret = print_hw_stat(stats, hsa->index, buf);
>> +unlock:
>> + mutex_unlock(&stats->lock);
>> +
>> + return ret;
>> }
>
> I think we are going to add a lock, it should cover all readers
> too. There is no real reason not too that I can see.
I'll protect setting/reading lifespan as well.
>
> So switch stats->lock to a rwlock and hold it in
Why rwlock?
seeing as setting/reading lifespan probably isn't going to be
used a lot (as opposed to reading a counter) and from the driver
point of view, reading a counter will usually mean "Do a FW query command"
I really don't think rwlock is the right option here.
>
> show_hw_stats (write)
> show_stats_lifespan
> set_stats_lifespan
>
> Which is all the places that touch stats..
>
> Jason
>
Mark
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
