On 3/26/2015 1:42 AM, Bart Van Assche wrote:
On 03/25/2015 02:19 PM, Somnath Kotur wrote:
+ if (cache->data_vec[ix].attr.ndev &&
+ cache->data_vec[ix].attr.ndev != old_net_dev)
A few lines earlier the memory old_net_dev points at was freed. If two
instances of this function run concurrently, what prevents that the
old_net_dev memory has been reallocated and hence that attr.ndev ==
old_net_dev although both pointers refer(red) to different network
devices ?
write_gid is *almost* always called in a mutex. The only case it's not
protected is in free_roce_gid_cache. free_roce_gid_cache is called only
in the error flow of roce_gid_cache_setup_one, when no concurrent
write_gid could be made (as the cache isn't setup yet) and in
roce_gid_cache_cleanup_one. roce_gid_cache_client_setup_one is called in
the error flow of roce_gid_cache_client_setup_one (where no other
write_gid are expected for the same above reason) and in
roce_gid_cache_client_cleanup_work_handler, where it's called after
flush_workqueue(roce_gid_mgmt_wq). Since all write_gids are called
through roce_gid_mgmt_wq and we set the cache to inactive mode before
flushing the wq and freeing the cache, I think we can conclude no
concurrent write_gid on the same cache are possible.
+ ACCESS_ONCE(cache->data_vec[ix].seq) = orig_seq;
Invoking write_gid() is only safe if the caller serializes write_gid()
calls. Apparently the cache->lock mutex is used for that purpose. So why
is it necessary to use ACCESS_ONCE() here ? Why is it needed to prevent
that the compiler coalesces this write with another write into the same
structure ?
The mutex only serializes cache writes. Cache reads could be done in
concurrent with writes and are protected by the ACCESS_ONCE.
+ /* Make sure the sequence number we remeber was read
This looks like a typo - shouldn't the above read "remember" ?
Will be fixed in V4, thanks.
BTW, the style of that comment is recommended only for networking code
and not for IB code. Have you verified this patch with checkpatch ?
Of course and I've just re-run checkpatch on this patch. It doesn't
catch this.
+ mutex_lock(&cache->lock);
+
+ for (ix = 0; ix < cache->sz; ix++)
+ if (cache->data_vec[ix].attr.ndev == ndev)
+ write_gid(ib_dev, port, cache, ix, &zgid, &zattr);
+
+ mutex_unlock(&cache->lock);
+ return 0;
The traditional Linux kernel coding style is one blank line before
mutex_lock() and after mutex_unlock() but not after mutex_lock() nor
before mutex_unlock().
I didn't find this in the CodingStyle doc. Could you please quote or
post a link?
+ orig_seq = ACCESS_ONCE(cache->data_vec[index].seq);
+ /* Make sure we read the sequence number before copying the
+ * gid to local storage. */
+ smp_rmb();
Please use READ_ONCE() instead of ACCESS_ONCE() as recommended in
<linux/compiler.h>.
Ok, I'll change that in V4. I see that READ_ONCE and WRITE_ONCE is
different from ACCESS_ONCE only for aggregated data types (which isn't
our case), but it won't hurt to change that.
+static void free_roce_gid_cache(struct ib_device *ib_dev, u8 port)
+{
+ int i;
+ struct ib_roce_gid_cache *cache =
+ ib_dev->cache.roce_gid_cache[port - 1];
+
+ if (!cache)
+ return;
+
+ for (i = 0; i < cache->sz; ++i) {
+ if (memcmp(&cache->data_vec[i].gid, &zgid,
+ sizeof(cache->data_vec[i].gid)))
+ write_gid(ib_dev, port, cache, i, &zgid, &zattr);
+ }
> + kfree(cache->data_vec);
> + kfree(cache);
> +}
Overwriting data just before it is freed is not useful. Please use
CONFIG_SLUB_DEBUG=y to debug use-after-free issues instead of such code.
It's mandatory as write_gid with zgid might cause the vendor driver to
free memory it allocated for this GID entry (like in the mlx4 case).
Bart.
Thanks for the review :)
Matan
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
