[nfs-discuss] Code review for 6672480: NFSv2/3 client panic when mounted with rsize=0

[Frank Batschulat (Home)]

On Fri, 13 Feb 2009 17:30:50 +0100, Marcel Telka  
wrote:

> 6672480 NFSv2 and NFSv3 client panic in nfs_async_inactive() when mounted 
> with rsize=0
>
> Problem description:
> 
>
> Commands like
> # mount -o vers=3,rsize=0  server:/export/dir /mnt
> or
> # mount -o vers=2,rsize=0  server:/export/dir /mnt
>
> will immediatelly panic the machine when kmem_flags is set.
> NFSv4 is not affected.
>
> Root cause:
> ---
>
> NFSv3:
>
> nfs3_mount() calls nfs_setopts() and this function failed (returned non zero).
> Then nfs3_mount() freed mi of a vnode using nfs_free_mi() call. After that the
> nfs3_mount() tried to release the vnode using vn_rele(). Unfortunatelly, the
> vn_rele() is accessing mi (previously freed) of the vnode.
>
> NFSv2:
>
> The root cause is same as for NFSv3, with one exception: All is happening in
> nfs_mount() instead of nfs3_mount().
>
> Detailed root cause analysis (>30k chars) is available at
> http://cr.opensolaris.org/~aragorn/6672480.analysis
>
> The fix:
> 
>
> The fix move VN_RELE() call before the nfs_free_mi() call in both nfs3_mount()
> and nfs_mount() functions. For example, the similar order of calls is already
> in nfs3rootvp().
>
> The webrev is available at .
> Please note it contains a lot of formatting fixes so as a starting point I
> recommend the real fix at 
> .

Nice work! looks good to me.

-- 
frankB

It is always possible to agglutinate multiple separate problems
into a single complex interdependent solution.
In most cases this is a bad idea.

[nfs-discuss] Code review for 6672480: NFSv2/3 client panic when mounted with rsize=0

[Marcel Telka]

Hi James,

On Fri, Feb 13, 2009 at 11:02:15AM -0600, james wahlig wrote:
> Your condensed explanation had me a little confused, how does vn_rele()  

Sorry for that, I apologize.

> access the mi?  However, I realize that is the async_inactive that uses  
> the vnode to get the mi.

Yes. That's true it is in nfs_async_inactive():

1881 void
1882 nfs_async_inactive(vnode_t *vp, cred_t *cr,
1883 void (*inactive)(vnode_t *, cred_t *, caller_context_t *))
1884 {
1885mntinfo_t *mi;
1886struct nfs_async_reqs *args;
1887 
1888mi = VTOMI(vp);
1889 

[... snip ...]

1910mutex_enter(&mi->mi_async_lock);  <--- HERE
1911if (mi->mi_manager_thread == NULL) {
1912rnode_t *rp = VTOR(vp);
1913 

To complete the information, the panic stack is:

> ::stack
vpanic(10bc768, 10bc788, 60010be14f0, deadbeefdeadbee8, 30007f9d840, 0)
nfs_async_inactive+0x44(60010be14f0, 60010b08be0, 7b796170, 5, 60010be1380, 
60010be3970)
nfs3_mount+0x9c8(0, 60010be1380, 60010315430, 6001145f300, 2a1006779d8, 0)
domount+0xac8(60010be3970, 2a1006779d8, 60011454600, 802c, 104, 0)
mount+0x108(600102924b8, 2a100677ad8, 31090, 2ec9c, 104, ffbffe94)
syscall_ap+0x58(2a0, ffbffeb0, 11516d4, 60010292400, 15, 4c)
syscall_trap32+0xcc(ffbffe94, ffbffeb0, 104, 2ec9c, 31090, 4c)


>
> The changes look good.

Thank you!

-- 
Marcel Telka
Solaris RPE

[nfs-discuss] Code review for 6672480: NFSv2/3 client panic when mounted with rsize=0

[Marcel Telka]

Hi Tom,

On Fri, Feb 13, 2009 at 10:51:58AM -0600, Tom Haynes wrote:
> Marcel Telka wrote:
>> The webrev is available at .
>> Please note it contains a lot of formatting fixes so as a starting point I
>> recommend the real fix at 
>> .
>>
>>   
>
> I would have preferred that the real fix also be a full webrev. It is  
> nice to be able to look at the full context.

Good idea. I'll do that in my next fix, I confess! :-)

Anyway, I hope you could easily locate the real fix in full webrev, if needed.

>
> BTW - thanks for thinking of isolating the changes.

You are welcome :-).

>
> Have you scanned for other instances of this cleanup code?

Yes. I scanned for all occurences of nfs_free_mi() - the dangerous function -
and I found nothing suspicious. There are only three instances:

nfs_mount()/nfs3_mount() - our case
nfsrootvp()/nfs3rootvp() - the case I am referring as an example of good usage
nfs_freevfs()/nfs3_freevfs() - here the mi is related to vfs, not vnode

>
> In any event, I approve of the changes.

Thank you!

-- 
Marcel Telka
Solaris RPE

[nfs-discuss] Code review for 6672480: NFSv2/3 client panic when mounted with rsize=0

[Marcel Telka]

Hi all,


The bug:


6672480 NFSv2 and NFSv3 client panic in nfs_async_inactive() when mounted with 
rsize=0


Problem description:


Commands like
# mount -o vers=3,rsize=0  server:/export/dir /mnt
or
# mount -o vers=2,rsize=0  server:/export/dir /mnt

will immediatelly panic the machine when kmem_flags is set.
NFSv4 is not affected.


Root cause:
---

NFSv3:

nfs3_mount() calls nfs_setopts() and this function failed (returned non zero).
Then nfs3_mount() freed mi of a vnode using nfs_free_mi() call. After that the
nfs3_mount() tried to release the vnode using vn_rele(). Unfortunatelly, the
vn_rele() is accessing mi (previously freed) of the vnode.

NFSv2:

The root cause is same as for NFSv3, with one exception: All is happening in
nfs_mount() instead of nfs3_mount().

Detailed root cause analysis (>30k chars) is available at
http://cr.opensolaris.org/~aragorn/6672480.analysis


The fix:


The fix move VN_RELE() call before the nfs_free_mi() call in both nfs3_mount()
and nfs_mount() functions. For example, the similar order of calls is already
in nfs3rootvp().

The webrev is available at .
Please note it contains a lot of formatting fixes so as a starting point I
recommend the real fix at .


The finale:
---

Please review my fix. Should you have any questions just ask.


Thank you.

-- 
Marcel Telka
Solaris RPE

[nfs-discuss] Code review for 6672480: NFSv2/3 client panic when mounted with rsize=0

[james wahlig]

Your condensed explanation had me a little confused, how does vn_rele() 
access the mi?  However, I realize that is the async_inactive that uses 
the vnode to get the mi.

The changes look good.

Jim

Marcel Telka wrote:

>Hi all,
>
>
>The bug:
>
>
>6672480 NFSv2 and NFSv3 client panic in nfs_async_inactive() when mounted with 
>rsize=0
>
>
>Problem description:
>
>
>Commands like
># mount -o vers=3,rsize=0  server:/export/dir /mnt
>or
># mount -o vers=2,rsize=0  server:/export/dir /mnt
>
>will immediatelly panic the machine when kmem_flags is set.
>NFSv4 is not affected.
>
>
>Root cause:
>---
>
>NFSv3:
>
>nfs3_mount() calls nfs_setopts() and this function failed (returned non zero).
>Then nfs3_mount() freed mi of a vnode using nfs_free_mi() call. After that the
>nfs3_mount() tried to release the vnode using vn_rele(). Unfortunatelly, the
>vn_rele() is accessing mi (previously freed) of the vnode.
>
>NFSv2:
>
>The root cause is same as for NFSv3, with one exception: All is happening in
>nfs_mount() instead of nfs3_mount().
>
>Detailed root cause analysis (>30k chars) is available at
>http://cr.opensolaris.org/~aragorn/6672480.analysis
>
>
>The fix:
>
>
>The fix move VN_RELE() call before the nfs_free_mi() call in both nfs3_mount()
>and nfs_mount() functions. For example, the similar order of calls is already
>in nfs3rootvp().
>
>The webrev is available at .
>Please note it contains a lot of formatting fixes so as a starting point I
>recommend the real fix at .
>
>
>The finale:
>---
>
>Please review my fix. Should you have any questions just ask.
>
>
>Thank you.
>
>  
>

[nfs-discuss] Code review for 6672480: NFSv2/3 client panic when mounted with rsize=0

[Tom Haynes]

Marcel Telka wrote:
> Hi all,
>
>
> The bug:
> 
>
> 6672480 NFSv2 and NFSv3 client panic in nfs_async_inactive() when mounted 
> with rsize=0
>
>
> Problem description:
> 
>
> Commands like
> # mount -o vers=3,rsize=0  server:/export/dir /mnt
> or
> # mount -o vers=2,rsize=0  server:/export/dir /mnt
>
> will immediatelly panic the machine when kmem_flags is set.
> NFSv4 is not affected.
>
>
> Root cause:
> ---
>
> NFSv3:
>
> nfs3_mount() calls nfs_setopts() and this function failed (returned non zero).
> Then nfs3_mount() freed mi of a vnode using nfs_free_mi() call. After that the
> nfs3_mount() tried to release the vnode using vn_rele(). Unfortunatelly, the
> vn_rele() is accessing mi (previously freed) of the vnode.
>
> NFSv2:
>
> The root cause is same as for NFSv3, with one exception: All is happening in
> nfs_mount() instead of nfs3_mount().
>
> Detailed root cause analysis (>30k chars) is available at
> http://cr.opensolaris.org/~aragorn/6672480.analysis
>
>
> The fix:
> 
>
> The fix move VN_RELE() call before the nfs_free_mi() call in both nfs3_mount()
> and nfs_mount() functions. For example, the similar order of calls is already
> in nfs3rootvp().
>
> The webrev is available at .
> Please note it contains a lot of formatting fixes so as a starting point I
> recommend the real fix at 
> .
>
>   

I would have preferred that the real fix also be a full webrev. It is 
nice to be able to look at the full context.

BTW - thanks for thinking of isolating the changes.

Have you scanned for other instances of this cleanup code?

In any event, I approve of the changes.

> The finale:
> ---
>
> Please review my fix. Should you have any questions just ask.
>
>
> Thank you.
>
>