On Thu, Mar 22, 2018 at 10:38:01AM +0200, Mark Bloch wrote:
>
>
> 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.
No specific reason, mutex too
Jason
--
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
