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 ?
+ 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 ?
+ /* Make sure the sequence number we remeber was read
This looks like a typo - shouldn't the above read "remember" ?
BTW, the style of that comment is recommended only for networking code
and not for IB code. Have you verified this patch with checkpatch ?
+ 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().
+ 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>.
+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.
Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
