[Ocfs2-devel] [PATCH 1/1] OCFS2: don't leave free'd mle attached to hb events

[Sunil Mushran]

oops... I read it incorrectly. But that does not sound right.
Let me think about it.

Wengang Wang wrote:
> Hi Sunil,
>
> Sunil Mushran wrote:
>> NAK
>>
>> wengang wang wrote:
>>> don't leave free'd mle attached to hb events.
>>>     in dlm_add_migration_mle() the mle is attched to "heartbeat 
>>> events" anyway no
>>> matter there is an existing mle with same name(returns -EEXIST).
>>> dlm_migrate_lockres() calls dlm_add_migration_mle(). in case the 
>>> later function
>>> returning -EEXIST, dlm_migrate_lockres() frees the (new) mle without 
>>> detaching
>>> it from "hb events". so that later "hb events" related operations 
>>> could improperly
>>> operate against wrong mle objects or against an invalid memory address.
>>
>> The mle is attached to hb events in dlm_init_mle() which is not called
>> if it returns -EEXIST. When it returns -EEXIST, oldmle is set to the
>> existing mle and its refcounting is handled correctly. mle is not 
>> touched
>> and thus only needs to be freed.
>>
>
> Maybe I am wrong. but are you sure dlm_init_mle() is not called when 
> it returns -EEXIST?
> in code, it doesn't return immediately after setting ret with -EEXIST, 
> but continue to call
> dlm_init_mle();
> and then
> mle->new_master = new_master;
>
> Maybe I am wrong somewhere?
> simplified code pasted here:
>
>
> 3098 static int dlm_add_migration_mle(struct dlm_ctxt *dlm,
> 3099                                  struct dlm_lock_resource *res,
> 3100                                  struct dlm_master_list_entry *mle,
> 3101                                  struct dlm_master_list_entry 
> **oldmle,
> 3102                                  const char *name, unsigned int 
> namelen,
> 3103                                  u8 new_master, u8 master)
> 3104 {
> 3116         found = dlm_find_mle(dlm, oldmle, (char *)name, namelen);
> 3117         if (found) {
> 3118                 struct dlm_master_list_entry *tmp = *oldmle;
> 3120                 if (tmp->type == DLM_MLE_MIGRATION) {
> 3121                         if (master == dlm->node_num) {
> 3126                                 ret = -EEXIST;
> 3127                         } else {
> 3136                                 BUG();
> 3137                         }
> 3138                 } else {
>                  ....
> 3151                 }
> 3153         }
> 3154
> 3156         dlm_init_mle(mle, DLM_MLE_MIGRATION, dlm, res, name, 
> namelen);
> 3157         mle->new_master = new_master;
>          ......
> 3163         __dlm_insert_mle(dlm, mle);
> 3164
> 3165         return ret;
> 3166 }
>
>
> regards,
> wengang.