Re: [PATCH for-next V1 2/3] IB/core: RoCE GID management separate cleanup and release

[Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2974 bytes --]

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 <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> 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 <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>

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 <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]