On 08/04/2015 05:23 PM, Jason Gunthorpe wrote: > On Tue, Aug 04, 2015 at 09:55:03PM +0300, Matan Barak wrote: > >> If it fails in ib_device_register_sysfs, the device release function >> isn't called and the device pointer isn't freed. > > Ugh, yes, the abuse in ib_dealloc_device needs to go too. > > Attached is a compile tested patch that fixes that up. Feel free to > fix it up as necessary. It should be sequenced before your 'Add RoCE > GID table management'.. It would probably be helpful for doug to > resend that one patch adjusted. > >> If ib_cache_setup_one fails, we call ib_device_unregister_sysfs and >> the memory will be freed. > > ?? ib_device_unregister_sysfs doesn't free memory.. > > At the driver the sequence should be: > ib_alloc_device > ib_register_device > ib_unregister_device > ib_dealloc_device > > The issue I'm looking (which I suspected before, but just went and > confirmed) at is that sysfs's show_port_gid calls ib_query_gid which > your series now makes use the cache, so sysfs should ideally be shut > off before the cache stuff. > > .. and we must setup the cache before setting up sysfs, otherwise > there is a race where userspace can trigger a cache call before > setup.. (null deref?) > >> ib_device_unregister_sysfs (otherwise I'll have use-after-free). >> What do you think? > > I'm still not sure what you are seeing.. > > You might also want the cache code to have an entry from > ib_alloc_device to setup locks and other no-fail initalization > things. AFIAK there isn't a strong reason to do this other than to > keep with the basic idiom. > > Jason > > From 32bafdef61a9e571ef1457f7f1966a7372d1b8d7 Mon Sep 17 00:00:00 2001 > From: Jason Gunthorpe <jguntho...@obsidianresearch.com> > Date: Tue, 4 Aug 2015 15:11:41 -0600 > Subject: [PATCH] IB/core: Make ib_alloc_device initialize the kobject > > This gets rid of the weird in-between state where struct ib_device > was allocated but the kobject didn't work. > > Consequently ib_device_release is now guaranteed to be called in > all situations and we can't duplicate its kfrees on error paths. > Choose to just drop kfree(port_immutable) > > Signed-off-by: Jason Gunthorpe <jguntho...@obsidianresearch.com>
I reworded the commit message some, but the patch was good and makes
sense. I've inserted it into the GID series and, as expected, it caused
failures in the original "IB/core: Add RoCE GID table management" patch.
However, the fixups for those failures seem pretty obvious to me, so I
made them myself. When I finally push this, it will be in this branch
on my github repo:
v4.2-rc6-based/gid-table-v7
You and Matan might want to double check that I fixed things up properly
and didn't miss something.
--
Doug Ledford <dledf...@redhat.com>
GPG KeyID: 0E572FDD
signature.asc
Description: OpenPGP digital signature
