2011/6/9 Pavel Shilovsky <piastryyy@xxxxxxxxx>:
> 2011/6/9 Christoph Hellwig <hch@xxxxxxxxxxxxx>:
>>> static void
>>> cifs_put_super(struct super_block *sb)
>>> {
>>> + struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
>>> + if (cifs_sb == NULL) {
>>> + cFYI(1, "Empty cifs superblock info passed to put_super");
>>> + return;
>>> + }
>>> +
>>> + bdi_destroy(&cifs_sb->bdi);
>>
>> This means you have a problem with the lifetime rules in cifs_do_mount.
>>
>> The VFS only calls ->put_super if sb->s_root is set. So the rules for
>> the mount handler are to only set s_root once the superblock is fully
>> set up.
>>
>> Also you should never call cifs_umount from the error handling in
>> cifs_do_mount. Until s_root is set, please unwind manually, after
>> that leave it to ->put_super.
>>
>>
>>> +
>>> +static void
>>> +cifs_kill_super(struct super_block *sb)
>>> +{
>>
>> This also seems quite broken. If you fix up the mount path like
>> I suggested it won't be nessecary.
>>
>>> int rc = 0;
>>> struct cifs_sb_info *cifs_sb;
>>>
>>> cFYI(1, "In cifs_put_super");
>>> cifs_sb = CIFS_SB(sb);
>>> if (cifs_sb == NULL) {
>>
>> And this check should also be removed.
>>
>>
>
> It seems to me that I didn't understand your points clearly.
>
> That's how I reproduce the problem:
> 1) one process does mount/umount calls in a loop;
> 2) another process does mount and umount calls.
>
> As the result, kernel crashes sometimes on mount of the second
> process, sometimes - on umount of the second process. The problem is
> that:
>
> one of these two processes process umount call. So, it comes to
> cifs_put_super, calls cifs_umount and then calls kfree(cifs_sb). In
> the same time, another process comes into cifs_do_mount and calls
> sget(). Then it appers into cifs_match_super that thinks that all
> structures like server, ses, tcon and cifs_sb are valid, but it is not
> true - the first process has already freed them but didn't remove
> superblock from fs_supers list.
>
> So, I simply reodered calls: now umount codepath marks a superblock as
> non-active and calls kill_sb(new cifs_kill_super), it calls
> kill_anon_super (that calls cifs_put_super and removes superblock from
> fs_supers list) and after that kill_sb frees all cifs structures
> (server, ses, tcon, cifs_sb).
>
> As the result, when we get into cifs_match_super (in mount from
> another process) we are sure that all cifs structures are valid.
>
> If I am not right, correct me, please!
>
> --
> Best regards,
> Pavel Shilovsky.
>
I also want to mention that NFS mount code uses the same order:
1) in moun at first it sets up server structure and then it calls sget.
2) it has put_super that only calls bdi_unregister and kill_super that
at first call kill_anon_super and then frees server structure.
--
Best regards,
Pavel Shilovsky.
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
