From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964837AbWHNUel (ORCPT ); Mon, 14 Aug 2006 16:34:41 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S964838AbWHNUel (ORCPT ); Mon, 14 Aug 2006 16:34:41 -0400 Received: from smtp-out.google.com ([216.239.45.12]:49558 "EHLO smtp-out.google.com") by vger.kernel.org with ESMTP id S964837AbWHNUek (ORCPT ); Mon, 14 Aug 2006 16:34:40 -0400 DomainKey-Signature: a=rsa-sha1; s=beta; d=google.com; c=nofws; q=dns; h=received:message-id:date:from:to:subject:cc:in-reply-to: mime-version:content-type:content-transfer-encoding: content-disposition:references; b=VhAOPKhRtHpbiHFpSNI6KPzmLLUUertNjd3+/2z604suwuXpumFB5anxJNtWDyDYV zAo714B/0C6Mzwfu1ywYw== Message-ID: Date: Mon, 14 Aug 2006 16:34:15 -0400 From: "Alex Polvi" To: "Trond Myklebust" Subject: Re: [PATCHv3] sunrpc/auth_gss: NULL pointer deref in gss_pipe_release() Cc: linux-kernel@vger.kernel.org In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <1154378242.13744.14.camel@localhost> <1155137983.5731.95.camel@localhost> Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org On 8/14/06, Alex Polvi wrote: > On 8/9/06, Trond Myklebust wrote: > > On Wed, 2006-08-09 at 11:27 -0400, Alex Polvi wrote: > > > On 7/31/06, Trond Myklebust wrote: > > > > On Mon, 2006-07-31 at 10:50 -0400, Alex Polvi wrote: > > > > > Proposed (trivial) patch to fix a NULL pointer deref in > > > > > gss_pipe_release(). While this does seem to fix the problem, I'm not > > > > > entirely sure it is the correct place to handle the NULL pointer. > > > > > > > > > > Included below is the script I used to recreate the problem, the oops, > > > > > and the patch. > > > > > > > > Sorry, but that is not the correct fix. The problem here is rather that > > > > something is causing us to call rpc_close_pipes() on the file after the > > > > call to gss_destroy(). That is supposed to be illegal. > > > > > > Since rpc_rmdir can potentially call rpc_pipe_release, here is a patch > > > to make sure it is not called after rpcauth_destroy (which calls > > > gss_destroy)! > > > > No. The current order is correct. The auth layer owns the RPCSEC_GSS > > pipe, not the nfs_client. > > > > rpc_rmdir() should only be calling rpc_pipe_release() if the auth layer > > fails to clean up after itself. > > Here is another fix. It is quite silly, but clnt->cl_auth is set to > NULL in rpc_destroy_client(), then eventually referenced in > gss_release_pipe() via rpc_rmdir(). Simply removing the clnt->cl_auth > = NULL from clnt.c fixes the issue. I'm still trying to understand the > subsystem, but it seems like rpc_rmdir is being correctly called to > clean up because of the weirdness with umount -l and the nfs server > being turned on and off. Does that seem correct? Or is this still just > covering up some other part of the code being sloppy cleaning up? Also, I just want to make it clear that I do not think this is the proper fix. It is just pointing out that we intentionally set cl_auth to NULL, then reference it. -Alex > Signed-off-by: Alex Polvi > -- > git diff net/sunrpc/clnt.c > diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c > index d6409e7..bbe2984 100644 > --- a/net/sunrpc/clnt.c > +++ b/net/sunrpc/clnt.c > @@ -312,10 +312,8 @@ rpc_destroy_client(struct rpc_clnt *clnt > > dprintk("RPC: destroying %s client for %s\n", > clnt->cl_protname, clnt->cl_server); > - if (clnt->cl_auth) { > + if (clnt->cl_auth) > rpcauth_destroy(clnt->cl_auth); > - clnt->cl_auth = NULL; > - } > if (clnt->cl_parent != clnt) { > rpc_destroy_client(clnt->cl_parent); > goto out_free; > > > > > > Signed-off-by: Alex Polvi > > > -- > > > diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c > > > index d6409e7..d2ff886 100644 > > > --- a/net/sunrpc/clnt.c > > > +++ b/net/sunrpc/clnt.c > > > @@ -312,16 +312,16 @@ rpc_destroy_client(struct rpc_clnt *clnt > > > > > > dprintk("RPC: destroying %s client for %s\n", > > > clnt->cl_protname, clnt->cl_server); > > > - if (clnt->cl_auth) { > > > - rpcauth_destroy(clnt->cl_auth); > > > - clnt->cl_auth = NULL; > > > - } > > > if (clnt->cl_parent != clnt) { > > > rpc_destroy_client(clnt->cl_parent); > > > goto out_free; > > > } > > > if (clnt->cl_pathname[0]) > > > rpc_rmdir(clnt->cl_pathname); > > > + if (clnt->cl_auth) { > > > + rpcauth_destroy(clnt->cl_auth); > > > + clnt->cl_auth = NULL; > > > + } > > > if (clnt->cl_xprt) { > > > xprt_destroy(clnt->cl_xprt); > > > clnt->cl_xprt = NULL; > > > > > > > > > > > > > > > > > > > > > > > polvi@return:~/sysops/experimental/polvi$ cat oopsmynfs.sh > > > > > #!/bin/bash > > > > > > > > > > cd / # make sure we are not in the dir > > > > > > > > > > PROG=${0##*/} > > > > > > > > > > function usage { > > > > > cat < > > > > usage: $PROG /nfs/path/ > > > > > > > > > > will oops sunrpc using the nfs host that serves /nfs/path/ > > > > > EOF > > > > > exit 1 > > > > > } > > > > > > > > > > DIR=$1 > > > > > > > > > > [ -d "$DIR" ] || usage > > > > > > > > > > sudo umount $DIR 2> /dev/null # just house-keeping... > > > > > > > > > > sudo mount -o sec=krb5 randomfiler:/vol/to/some/share $DIR # must use krb5 > > > > > > > > > > # with out this ls the cd below will say permission denied and not > > > > > hang after the > > > > > # nfs server has been turned off. It has something to do with stat64 > > > > > returning EACCES > > > > > ls $DIR > /dev/null > > > > > > > > > > echo "Make the nfs server unusable by the client (turn off nfs, iptables, etc)." > > > > > read -p "press enter when ready" > > > > > > > > > > # if the echo is hit, this script failed to oops sunrpc > > > > > (cd $DIR/. ; echo "will not cause an oops") & # this should hang > > > > > > > > > > sudo umount -l $DIR > > > > > > > > > > echo "Turn the nfs server back on and watch for the oops. Should not > > > > > take more then 10s" > > > > > > > > > > > > > > > [ 204.385339] net/sunrpc/rpc_pipe.c: rpc_lookup_parent failed to find > > > > > path /nfs/clnt4/krb5 > > > > > [ 204.385427] BUG: unable to handle kernel NULL pointer dereference > > > > > at virtual address 0000006c > > > > > [ 204.385554] printing eip: > > > > > [ 204.385595] c01d2322 > > > > > [ 204.385678] *pde = 00000000 > > > > > [ 204.385719] Oops: 0000 [#1] > > > > > [ 204.385781] SMP > > > > > [ 204.385875] Modules linked in: des binfmt_misc autofs4 video button > > > > > battery ac nfs lockd af_packet sg sr_mod pcspkr rtc psm > > > > > > > > > > > > > > > ouse mousedev ide_disk ide_cd cdrom > > > > > rpcsec_gss_krb5 auth_rpcgss sunrpc ext3 jbd mbcache thermal processor > > > > > fan tg3 sd_mod ide_g > > > > > > > > > > > > > > > eneric ata_piix libata scsi_mod generic ide_core unix > > > > > [ 204.387417] CPU: 0 > > > > > [ 204.387418] EIP: 0060:[] Not tainted VLI > > > > > [ 204.387419] EFLAGS: 00010292 (2.6.18-rc2-git #1) > > > > > [ 204.387538] EIP is at _raw_spin_lock+0x12/0x170 > > > > > [ 204.387578] eax: 00000068 ebx: 00000068 ecx: f7157d3c edx: f7157d3c > > > > > [ 204.387621] esi: 00000068 edi: 00000000 ebp: f7157d00 esp: f7157cdc > > > > > [ 204.387664] ds: 007b es: 007b ss: 0068 > > > > > [ 204.387704] Process oopsmynfs.sh (pid: 6114, ti=f7156000 > > > > > task=dffb5aa0 task.ti=f7156000) > > > > > [ 204.387748] Stack: dffb5aa0 f7157d1c c029ee7d f7157cfc f7156000 > > > > > f7156000 f73acd00 00000068 > > > > > [ 204.388040] 00000000 f7157d0c c029ff7e 00000068 f7157d24 > > > > > f88d82cf f73acd00 f73acd00 > > > > > [ 204.388332] f73acecc f88e1554 f7157d50 f8cbd6a9 f73acd00 > > > > > f7288460 f73acd80 f73acd70 > > > > > [ 204.388625] Call Trace: > > > > > [ 204.388868] [] _spin_lock+0xe/0x10 > > > > > [ 204.388997] [] gss_pipe_release+0x1f/0x70 [auth_rpcgss] > > > > > [ 204.389075] [] rpc_close_pipes+0xe9/0x130 [sunrpc] > > > > > [ 204.389173] [] rpc_depopulate+0xff/0x140 [sunrpc] > > > > > [ 204.389267] [] rpc_rmdir+0x6d/0xa0 [sunrpc] > > > > > [ 204.389360] [] rpc_destroy_client+0xde/0x110 [sunrpc] > > > > > [ 204.389443] [] rpc_destroy_client+0x61/0x110 [sunrpc] > > > > > [ 204.389524] [] rpc_shutdown_client+0xb7/0x120 [sunrpc] > > > > > [ 204.389605] [] nfs_kill_super+0x3b/0x90 [nfs] > > > > > [ 204.389692] [] deactivate_super+0x81/0xa0 > > > > > [ 204.389858] [] mntput_no_expire+0x52/0x90 > > > > > [ 204.390039] [] path_release+0x2a/0x30 > > > > > [ 204.390211] [] vfs_stat_fd+0x4b/0x60 > > > > > [ 204.390378] [] vfs_stat+0x20/0x30 > > > > > [ 204.390545] [] sys_stat64+0x19/0x30 > > > > > [ 204.390712] [] sysenter_past_esp+0x56/0x79 > > > > > [ 204.390787] [] 0xb7fff410 > > > > > [ 204.390854] Code: 2b c0 89 f8 e8 00 fe ff ff e9 14 ff ff ff 8d 74 > > > > > 26 00 8d bc 27 00 00 00 00 55 89 e5 83 ec 24 89 5d f4 8b > > > > > > > > > > > > > > > 5d 08 89 75 f8 89 7d fc <81> 7b > > > > > 04 ad 4e ad de 75 4c 89 e0 25 00 e0 ff ff 8b 00 39 43 0c > > > > > [ 204.392662] EIP: [] _raw_spin_lock+0x12/0x170 SS:ESP 0068:f7157cdc > > > > > > > > > > Signed-off-by: Alex Polvi > > > > > --- > > > > > diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c > > > > > index 4a9aa93..2db3bd1 100644 > > > > > --- a/net/sunrpc/auth_gss/auth_gss.c > > > > > +++ b/net/sunrpc/auth_gss/auth_gss.c > > > > > @@ -607,6 +607,9 @@ gss_pipe_release(struct inode *inode) > > > > > struct rpc_auth *auth; > > > > > struct gss_auth *gss_auth; > > > > > > > > > > + if (rpci->ops == NULL) > > > > > + return; > > > > > + > > > > > clnt = rpci->private; > > > > > auth = clnt->cl_auth; > > > > > gss_auth = container_of(auth, struct gss_auth, rpc_auth); > > > > > - > > > > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > > > > the body of a message to majordomo@vger.kernel.org > > > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > Please read the FAQ at http://www.tux.org/lkml/ > > > > > > > > > > > > >