This patchset fixes a couple of problems with the use-gss-proxy procfile
and does some general cleanup around that code. Bruce, can you consider
this for 3.14? Getting it into linux-next soon would be good too...
Thanks,
Jeff Layton (3):
sunrpc: don't wait for write before allowing reads from use-gss-proxy
file
sunrpc: fix potential race between setting use_gss_proxy and the
upcall rpc_clnt
sunrpc: get rid of use_gssp_lock
net/sunrpc/auth_gss/gss_rpc_upcall.c | 2 -
net/sunrpc/auth_gss/svcauth_gss.c | 75 ++++++++++--------------------------
net/sunrpc/netns.h | 1 -
3 files changed, 20 insertions(+), 58 deletions(-)
--
1.8.4.2
On Sat, Jan 04, 2014 at 09:11:05AM -0500, Jeff Layton wrote:
> On Sat, 4 Jan 2014 07:18:04 -0500
> Jeff Layton <[email protected]> wrote:
>
> > Currently, the write_gssp code will change the variable and wake up any
> > waiters waiting on that change. It then goes and tries to set the
> > gssp_clnt. This is racy -- a task waiting on the set_gss_proxy call may
> > end up waking up and then subsequently finding that the gss_clnt isn't
> > there yet and end up not using it even though it'll soon be ready.
> >
> > This patch reverses the order of operations. The gssp_clnt is created
> > first, and the variable change is done only if that succeeds.
> >
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > net/sunrpc/auth_gss/svcauth_gss.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> > index 1b94a9c..60dc370 100644
> > --- a/net/sunrpc/auth_gss/svcauth_gss.c
> > +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> > @@ -1317,10 +1317,10 @@ static ssize_t write_gssp(struct file *file, const char __user *buf,
> > return res;
> > if (i != 1)
> > return -EINVAL;
> > - res = set_gss_proxy(net, 1);
> > + res = set_gssp_clnt(net);
> > if (res)
> > return res;
> > - res = set_gssp_clnt(net);
> > + res = set_gss_proxy(net, 1);
> > if (res)
> > return res;
> > return count;
>
> Sorry, I forgot to update the patch description on this one. There is
> still a race here after patch #1, but it goes something like this:
>
> A nfsd thread will call use_gss_proxy and find it set to '1'. It'll
> then go and attempt and upcall, but since gssp_clnt is still NULL,
> gssp_call will just return -EIO.
>
> The patch is still the same however. Bruce, let me know if you want me
> to resend with a fixed commit msg.
No problem. Applying as follows.--b.
commit 32d6805adfc998def6b77ab95f35f63ad07cd043
Author: Jeff Layton <[email protected]>
Date: Sat Jan 4 07:18:04 2014 -0500
sunrpc: fix potential race between setting use_gss_proxy and the upcall rpc_clnt
An nfsd thread can call use_gss_proxy and find it set to '1' but find
gssp_clnt still NULL, so that when it attempts the upcall the result
will be an unnecessary -EIO.
So, ensure that gssp_clnt is created first, and set the use_gss_proxy
variable only if that succeeds.
Signed-off-by: Jeff Layton <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 084c87e..a80af65 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1317,10 +1317,10 @@ static ssize_t write_gssp(struct file *file, const char __user *buf,
return res;
if (i != 1)
return -EINVAL;
- res = set_gss_proxy(net, 1);
+ res = set_gssp_clnt(net);
if (res)
return res;
- res = set_gssp_clnt(net);
+ res = set_gss_proxy(net, 1);
if (res)
return res;
return count;
On Sat, Jan 04, 2014 at 07:18:02AM -0500, Jeff Layton wrote:
> This patchset fixes a couple of problems with the use-gss-proxy procfile
> and does some general cleanup around that code. Bruce, can you consider
> this for 3.14? Getting it into linux-next soon would be good too...
Yep, applied locally, should get pushed out tomorrow-ish.
I suppose the first should also go to stable if the "tar up /proc" case
merits it.
The races look like they require pretty unusual userspace behavior, so
I'm not inclined to backport those.
--b.
>
> Thanks,
>
> Jeff Layton (3):
> sunrpc: don't wait for write before allowing reads from use-gss-proxy
> file
> sunrpc: fix potential race between setting use_gss_proxy and the
> upcall rpc_clnt
> sunrpc: get rid of use_gssp_lock
>
> net/sunrpc/auth_gss/gss_rpc_upcall.c | 2 -
> net/sunrpc/auth_gss/svcauth_gss.c | 75 ++++++++++--------------------------
> net/sunrpc/netns.h | 1 -
> 3 files changed, 20 insertions(+), 58 deletions(-)
>
> --
> 1.8.4.2
>
On Sun, 5 Jan 2014 15:30:45 -0500
"J. Bruce Fields" <[email protected]> wrote:
> On Sat, Jan 04, 2014 at 07:18:02AM -0500, Jeff Layton wrote:
> > This patchset fixes a couple of problems with the use-gss-proxy procfile
> > and does some general cleanup around that code. Bruce, can you consider
> > this for 3.14? Getting it into linux-next soon would be good too...
>
> Yep, applied locally, should get pushed out tomorrow-ish.
>
> I suppose the first should also go to stable if the "tar up /proc" case
> merits it.
>
> The races look like they require pretty unusual userspace behavior, so
> I'm not inclined to backport those.
>
> --b.
>
Yeah, I think the first patch is probably reasonable for stable. There
are a number of tools that tar up /proc files in order to collect info
for troubleshooting and those are likely to get stuck with this.
The other two are theoretical races I noticed by inspection so I don't
think they would be appropriate for it.
Thanks,
--
Jeff Layton <[email protected]>
We can achieve the same result with a cmpxchg(). This also fixes a
potential race in use_gss_proxy(). The value of sn->use_gss_proxy could
go from -1 to 1 just after we check it in use_gss_proxy() but before we
acquire the spinlock. The procfile write would end up returning success
but the value would flip to 0 soon afterward. With this method we not
only avoid locking but the first "setter" always wins.
Signed-off-by: Jeff Layton <[email protected]>
---
net/sunrpc/auth_gss/svcauth_gss.c | 42 +++++++++++++++++----------------------
1 file changed, 18 insertions(+), 24 deletions(-)
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 60dc370..2a93540 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1263,41 +1263,35 @@ out:
return ret;
}
-DEFINE_SPINLOCK(use_gssp_lock);
-
-static bool use_gss_proxy(struct net *net)
+/*
+ * Try to set the sn->use_gss_proxy variable to a new value. We only allow
+ * it to be changed if it's currently undefined (-1). If it's any other value
+ * then return -EBUSY unless the type wouldn't have changed anyway.
+ */
+static int set_gss_proxy(struct net *net, int type)
{
struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
+ int ret;
- if (sn->use_gss_proxy != -1)
- return sn->use_gss_proxy;
- spin_lock(&use_gssp_lock);
- /*
- * If you wanted gss-proxy, you should have said so before
- * starting to accept requests:
- */
- sn->use_gss_proxy = 0;
- spin_unlock(&use_gssp_lock);
+ WARN_ON_ONCE(type != 0 && type != 1);
+ ret = cmpxchg(&sn->use_gss_proxy, -1, type);
+ if (ret != -1 && ret != type)
+ return -EBUSY;
return 0;
}
-#ifdef CONFIG_PROC_FS
-
-static int set_gss_proxy(struct net *net, int type)
+static bool use_gss_proxy(struct net *net)
{
struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
- int ret = 0;
- WARN_ON_ONCE(type != 0 && type != 1);
- spin_lock(&use_gssp_lock);
- if (sn->use_gss_proxy == -1 || sn->use_gss_proxy == type)
- sn->use_gss_proxy = type;
- else
- ret = -EBUSY;
- spin_unlock(&use_gssp_lock);
- return ret;
+ /* If use_gss_proxy is still undefined, then try to disable it */
+ if (sn->use_gss_proxy == -1)
+ set_gss_proxy(net, 0);
+ return sn->use_gss_proxy;
}
+#ifdef CONFIG_PROC_FS
+
static ssize_t write_gssp(struct file *file, const char __user *buf,
size_t count, loff_t *ppos)
{
--
1.8.4.2
On Sun, 5 Jan 2014 15:21:57 -0500
"J. Bruce Fields" <[email protected]> wrote:
> On Sat, Jan 04, 2014 at 09:11:05AM -0500, Jeff Layton wrote:
> > On Sat, 4 Jan 2014 07:18:04 -0500
> > Jeff Layton <[email protected]> wrote:
> >
> > > Currently, the write_gssp code will change the variable and wake up any
> > > waiters waiting on that change. It then goes and tries to set the
> > > gssp_clnt. This is racy -- a task waiting on the set_gss_proxy call may
> > > end up waking up and then subsequently finding that the gss_clnt isn't
> > > there yet and end up not using it even though it'll soon be ready.
> > >
> > > This patch reverses the order of operations. The gssp_clnt is created
> > > first, and the variable change is done only if that succeeds.
> > >
> > > Signed-off-by: Jeff Layton <[email protected]>
> > > ---
> > > net/sunrpc/auth_gss/svcauth_gss.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> > > index 1b94a9c..60dc370 100644
> > > --- a/net/sunrpc/auth_gss/svcauth_gss.c
> > > +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> > > @@ -1317,10 +1317,10 @@ static ssize_t write_gssp(struct file *file, const char __user *buf,
> > > return res;
> > > if (i != 1)
> > > return -EINVAL;
> > > - res = set_gss_proxy(net, 1);
> > > + res = set_gssp_clnt(net);
> > > if (res)
> > > return res;
> > > - res = set_gssp_clnt(net);
> > > + res = set_gss_proxy(net, 1);
> > > if (res)
> > > return res;
> > > return count;
> >
> > Sorry, I forgot to update the patch description on this one. There is
> > still a race here after patch #1, but it goes something like this:
> >
> > A nfsd thread will call use_gss_proxy and find it set to '1'. It'll
> > then go and attempt and upcall, but since gssp_clnt is still NULL,
> > gssp_call will just return -EIO.
> >
> > The patch is still the same however. Bruce, let me know if you want me
> > to resend with a fixed commit msg.
>
> No problem. Applying as follows.--b.
>
> commit 32d6805adfc998def6b77ab95f35f63ad07cd043
> Author: Jeff Layton <[email protected]>
> Date: Sat Jan 4 07:18:04 2014 -0500
>
> sunrpc: fix potential race between setting use_gss_proxy and the upcall rpc_clnt
>
> An nfsd thread can call use_gss_proxy and find it set to '1' but find
> gssp_clnt still NULL, so that when it attempts the upcall the result
> will be an unnecessary -EIO.
>
> So, ensure that gssp_clnt is created first, and set the use_gss_proxy
> variable only if that succeeds.
>
> Signed-off-by: Jeff Layton <[email protected]>
> Signed-off-by: J. Bruce Fields <[email protected]>
>
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> index 084c87e..a80af65 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -1317,10 +1317,10 @@ static ssize_t write_gssp(struct file *file, const char __user *buf,
> return res;
> if (i != 1)
> return -EINVAL;
> - res = set_gss_proxy(net, 1);
> + res = set_gssp_clnt(net);
> if (res)
> return res;
> - res = set_gssp_clnt(net);
> + res = set_gss_proxy(net, 1);
> if (res)
> return res;
> return count;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Looks good. Thanks for fixing that up...
--
Jeff Layton <[email protected]>
It doesn't make much sense to make reads from this procfile hang. As
far as I can tell, only gssproxy itself will open this file and it
never reads from it. Change it to just give the present setting of
sn->use_gss_proxy without waiting for anything.
Note that we do not want to call use_gss_proxy() in this codepath
since an inopportune read of this file could cause it to be disabled
prematurely.
Signed-off-by: Jeff Layton <[email protected]>
---
net/sunrpc/auth_gss/gss_rpc_upcall.c | 2 --
net/sunrpc/auth_gss/svcauth_gss.c | 33 ++-------------------------------
net/sunrpc/netns.h | 1 -
3 files changed, 2 insertions(+), 34 deletions(-)
diff --git a/net/sunrpc/auth_gss/gss_rpc_upcall.c b/net/sunrpc/auth_gss/gss_rpc_upcall.c
index 458f85e..abbb7dc 100644
--- a/net/sunrpc/auth_gss/gss_rpc_upcall.c
+++ b/net/sunrpc/auth_gss/gss_rpc_upcall.c
@@ -137,7 +137,6 @@ void init_gssp_clnt(struct sunrpc_net *sn)
{
mutex_init(&sn->gssp_lock);
sn->gssp_clnt = NULL;
- init_waitqueue_head(&sn->gssp_wq);
}
int set_gssp_clnt(struct net *net)
@@ -154,7 +153,6 @@ int set_gssp_clnt(struct net *net)
sn->gssp_clnt = clnt;
}
mutex_unlock(&sn->gssp_lock);
- wake_up(&sn->gssp_wq);
return ret;
}
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 008cdad..1b94a9c 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1295,34 +1295,9 @@ static int set_gss_proxy(struct net *net, int type)
else
ret = -EBUSY;
spin_unlock(&use_gssp_lock);
- wake_up(&sn->gssp_wq);
return ret;
}
-static inline bool gssp_ready(struct sunrpc_net *sn)
-{
- switch (sn->use_gss_proxy) {
- case -1:
- return false;
- case 0:
- return true;
- case 1:
- return sn->gssp_clnt;
- }
- WARN_ON_ONCE(1);
- return false;
-}
-
-static int wait_for_gss_proxy(struct net *net, struct file *file)
-{
- struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
-
- if (file->f_flags & O_NONBLOCK && !gssp_ready(sn))
- return -EAGAIN;
- return wait_event_interruptible(sn->gssp_wq, gssp_ready(sn));
-}
-
-
static ssize_t write_gssp(struct file *file, const char __user *buf,
size_t count, loff_t *ppos)
{
@@ -1355,16 +1330,12 @@ static ssize_t read_gssp(struct file *file, char __user *buf,
size_t count, loff_t *ppos)
{
struct net *net = PDE_DATA(file_inode(file));
+ struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
unsigned long p = *ppos;
char tbuf[10];
size_t len;
- int ret;
-
- ret = wait_for_gss_proxy(net, file);
- if (ret)
- return ret;
- snprintf(tbuf, sizeof(tbuf), "%d\n", use_gss_proxy(net));
+ snprintf(tbuf, sizeof(tbuf), "%d\n", sn->use_gss_proxy);
len = strlen(tbuf);
if (p >= len)
return 0;
diff --git a/net/sunrpc/netns.h b/net/sunrpc/netns.h
index 779742c..3a260e4 100644
--- a/net/sunrpc/netns.h
+++ b/net/sunrpc/netns.h
@@ -26,7 +26,6 @@ struct sunrpc_net {
unsigned int rpcb_is_af_local : 1;
struct mutex gssp_lock;
- wait_queue_head_t gssp_wq;
struct rpc_clnt *gssp_clnt;
int use_gss_proxy;
int pipe_version;
--
1.8.4.2
On Sat, 4 Jan 2014 07:18:04 -0500
Jeff Layton <[email protected]> wrote:
> Currently, the write_gssp code will change the variable and wake up any
> waiters waiting on that change. It then goes and tries to set the
> gssp_clnt. This is racy -- a task waiting on the set_gss_proxy call may
> end up waking up and then subsequently finding that the gss_clnt isn't
> there yet and end up not using it even though it'll soon be ready.
>
> This patch reverses the order of operations. The gssp_clnt is created
> first, and the variable change is done only if that succeeds.
>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> net/sunrpc/auth_gss/svcauth_gss.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> index 1b94a9c..60dc370 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -1317,10 +1317,10 @@ static ssize_t write_gssp(struct file *file, const char __user *buf,
> return res;
> if (i != 1)
> return -EINVAL;
> - res = set_gss_proxy(net, 1);
> + res = set_gssp_clnt(net);
> if (res)
> return res;
> - res = set_gssp_clnt(net);
> + res = set_gss_proxy(net, 1);
> if (res)
> return res;
> return count;
Sorry, I forgot to update the patch description on this one. There is
still a race here after patch #1, but it goes something like this:
A nfsd thread will call use_gss_proxy and find it set to '1'. It'll
then go and attempt and upcall, but since gssp_clnt is still NULL,
gssp_call will just return -EIO.
The patch is still the same however. Bruce, let me know if you want me
to resend with a fixed commit msg.
Thanks,
--
Jeff Layton <[email protected]>
Currently, the write_gssp code will change the variable and wake up any
waiters waiting on that change. It then goes and tries to set the
gssp_clnt. This is racy -- a task waiting on the set_gss_proxy call may
end up waking up and then subsequently finding that the gss_clnt isn't
there yet and end up not using it even though it'll soon be ready.
This patch reverses the order of operations. The gssp_clnt is created
first, and the variable change is done only if that succeeds.
Signed-off-by: Jeff Layton <[email protected]>
---
net/sunrpc/auth_gss/svcauth_gss.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 1b94a9c..60dc370 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1317,10 +1317,10 @@ static ssize_t write_gssp(struct file *file, const char __user *buf,
return res;
if (i != 1)
return -EINVAL;
- res = set_gss_proxy(net, 1);
+ res = set_gssp_clnt(net);
if (res)
return res;
- res = set_gssp_clnt(net);
+ res = set_gss_proxy(net, 1);
if (res)
return res;
return count;
--
1.8.4.2