gmane.linux.nfs

Re: Is the code stateid_generation_after()in legal C?

Joakim Tjernlund — 2013-05-18T12:10:08

Jim Rees <rees-63aXycvo3TyHXe+LvDLADg< at >public.gmane.org> wrote on 2013/05/17 18:50:38:
not

Right, the below test prog demonstrates the signed overflow problem in 
newer gcc's
#include <assert.h>

int foo(int a) {
  assert(a+100 > a);
  printf("%d %d\n",a+100,a);
  return a;
}

int main() {
  foo(100);
  foo(0x7fffffff);
}

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA< at >public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] nfsd: avoid undefined signed overflow

Bernd Petrovitsch — 2013-05-18T11:04:00

Hmm, what is actually wrong with plain-old
----  snip  ----
static bool stateid_generation_after(stateid_t *a, stateid_t *b)
{
return a->si_generation > b->si_generation;
}
----  snip  ----
?

Kind regards,
Bernd

Re: [PATCH v2] nfsd: avoid undefined signed overflow

Jim Rees — 2013-05-17T22:30:56

  On Fri, May 17, 2013 at 05:33:00PM -0400, Jim Rees wrote:
  > In C, signed integer overflow results in undefined behavior, but unsigned
  > overflow wraps around. So do the subtraction first, then cast to signed.

  Thanks!  Applying for 3.11.

  (I wonder if it actually bites anyone in practice?  Are there common
  compilers or architectures where this makes a difference?  Even if so I
  suppose that many generations is probably unlikely enough not to make
  this worth backporting to stable branches.)

I tested on the architectures I have at my fingertips and it made no
difference. My guess is you might need a one's complement architecture. Of
course we'll never see wraparound in our lifetimes, so no sense backporting
this.

I actually used a one's complement machine many years ago, a CDC 6600. I
don't think linux has been ported to it.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA< at >public.gmane.org
More majordomo i

Re: [PATCH v2] nfsd: avoid undefined signed overflow

Bruce Fields — 2013-05-17T21:40:31

Thanks!  Applying for 3.11.

(I wonder if it actually bites anyone in practice?  Are there common
compilers or architectures where this makes a difference?  Even if so I
suppose that many generations is probably unlikely enough not to make
this worth backporting to stable branches.)

--b.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA< at >public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] nfsd: avoid undefined signed overflow

Jim Rees — 2013-05-17T21:33:00

In C, signed integer overflow results in undefined behavior, but unsigned
overflow wraps around. So do the subtraction first, then cast to signed.

Suggested-by: Joakim Tjernlund <joakim.tjernlund-SNLAxHN9vbcOP4wsBPIw7w< at >public.gmane.org>
Signed-off-by: Jim Rees <rees-63aXycvo3TyHXe+LvDLADg< at >public.gmane.org>
---
 fs/nfsd/nfs4state.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 316ec84..9850329 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
< at >< at > -3427,7 +3427,7 < at >< at > grace_disallows_io(struct net *net, struct inode *inode)
 /* Returns true iff a is later than b: */
 static bool stateid_generation_after(stateid_t *a, stateid_t *b)
 {
-return (s32)a->si_generation - (s32)b->si_generation > 0;
+return (s32)(a->si_generation - b->si_generation) > 0;
 }
 
 static __be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session)

[PATCH] nfsd: avoid undefined signed overflow

Jim Rees — 2013-05-17T20:16:19

In C, signed integer overflow results in undefined behavior, but unsigned
overflow wraps around. So do the subtraction first, then cast to signed.

Signed-off-by: Jim Rees <rees-63aXycvo3TyHXe+LvDLADg< at >public.gmane.org>
---
 fs/nfsd/nfs4state.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 316ec84..9850329 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
< at >< at > -3427,7 +3427,7 < at >< at > grace_disallows_io(struct net *net, struct inode *inode)
 /* Returns true iff a is later than b: */
 static bool stateid_generation_after(stateid_t *a, stateid_t *b)
 {
-return (s32)a->si_generation - (s32)b->si_generation > 0;
+return (s32)(a->si_generation - b->si_generation) > 0;
 }
 
 static __be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session)

Re: [PATCH 2/3] SUNRPC: Faster detection if gssd is actually running

Myklebust, Trond — 2013-05-17T17:52:45

The timeout has to be non-zero, otherwise if you _do_ restart rpc.gssd,
it needs a certain time to actually connect to one of the gssd
rpc_pipes.

The 15 second initial timeout is there in order to deal with the fact
that it may take a moment or 2 for init to get round to starting
rpc.gssd. I didn't want to change that value right now.

Re: [PATCH 3/3] SUNRPC: Convert auth_gss pipe detection to work in namespaces

Myklebust, Trond — 2013-05-17T17:55:28

Possibly, however given that we're still seeing distros using the binary
mount interface that was replaced 6 years ago...

Re: Is the code stateid_generation_after()in legal C?

Jim Rees — 2013-05-17T16:50:38

  static bool stateid_generation_after(stateid_t *a, stateid_t *b)
  {
          return (s32)a->si_generation - (s32)b->si_generation > 0;
  }
  
  overflow is undefined for signed integers and gcc uses that nowadays.
  Not sure if that can affect the above code?

I guess the intent there is to account for stateid wraparound. But it's not
clear to me this is doing the right thing. I think C specifies overflow
behavior for unsigned but not signed. So shouldn't it be something more like
this?

    (s32)(a->si_generation - b->si_generation) > 0

Either way, this probably deserves a comment.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA< at >public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Is the code stateid_generation_after()in legal C?

Joakim Tjernlund — 2013-05-17T14:38:39

static bool stateid_generation_after(stateid_t *a, stateid_t *b)
{
        return (s32)a->si_generation - (s32)b->si_generation > 0;
}

overflow is undefined for signed integers and gcc uses that nowadays.
Not sure if that can affect the above code?

 Jocke
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA< at >public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Where in the server code is fsinfo rtpref calculated?

J. Bruce Fields — 2013-05-17T13:56:47

Maybe.  In any case I'd want to see data before changing our defaults.


Do you have a performance problem that you've actually measured, and if
so could you share the details?


I'm not terribly familiar with the client logic, but would expect this
to vary depending on kernel version, read-ahead policy, application
behavior and a number of other factors, so I'd recommend testing with
your workload and finding out.

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA< at >public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: NFS loop on 3.4.39

Joakim Tjernlund — 2013-05-17T13:18:54

Back on this old problem now with some more input. Upgraded my client to 
3.8.13(server is  on 3.4.44)
and got the NFS4ERR_OLD_STATEID ping pong effect.
Firefox was locked with process state "Dl" and I got this dmesg:

INFO: task mozStorage #2:6355 blocked for more than 120 seconds.
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
mozStorage #2   D f5619280     0  6355      1 0x00000000
 f446d440 00000086 f446d65c f5619280 f3745300 f5619280 ef0da810 c17e4280
 f3745300 c15a897e c17e4280 c17e4280 ef0da810 c17e4280 ef0da810 c17e4280
 f3745300 f3745300 c17e4280 c17e4280 ef0da810 c17e4280 f46124f8 f46124f8
Call Trace:
 [<c15a897e>] ? __schedule+0x23e/0x660
 [<c1088d70>] ? ktime_get_ts+0xf0/0x120
 [<c15a907e>] ? io_schedule+0x6e/0xb0
 [<c10c35f5>] ? sleep_on_page+0x5/0x10
 [<c15a75b5>] ? __wait_on_bit+0x45/0x70
 [<c10c35f0>] ? __lock_page+0x80/0x80
 [<c10c383d>] ? wait_on_page_bit+0x8d/0xa0
 [<c106ec10>] ? autoremove_wake_function+0x40/0x40
 [<c10c3a7c>] ? grab_cache_page_write_begin+0x9c/

Re: Where in the server code is fsinfo rtpref calculated?

James Vanns — 2013-05-17T11:43:02

Possibly. But we have no control over that (the application read size,
I mean).


No. But 32x 32k reads is reasonable (because it gives other RPCs a
look-in). I'm focused on reads because it makes up the majority of
our NFS traffic. I'm concerned because as it stands (out of the box)
if the majority of our n knfsd threads are waiting for a 1MB read to
return then no other RPC request will be serviced and will just
contribute to the backlog. This backlog itself will probably also contain
a hefty no. of 1MB read requests too. In short, a lot of other RPC calls
that are not reads will just be blocking and this will appear to an end
user as poor performance.

We deal with a great number of fairly large files - 10s of GBs in size. We just
don't want others to suffer because of large request sizes coming in (writes
end up being of the same size too but there are less of them). Our use cases are
varied but they all have to share the same resource (the array of NFS servers).

We've only really seen this since our u

Re: [PATCH 2/3] SUNRPC: Faster detection if gssd is actually running

J. Bruce Fields — 2013-05-17T01:03:44

Chuck points out I'm confused, it's actually 15s then 1/4s (why 1/4s?).

(Apologies, somehow I saw a ">> 2" in there and my brain shut down and
jumped to the "exponential delay" conclusion.)

Still sort of curious how we're choosing these delays, though.

Hard to imagine people won't still notice the delay on first mount.
Given there's a workaround (run gssd), maybe that's just good enough for
now, I don't know.

--b.


--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA< at >public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] clnt_vc_control: fix VERS/PROG size

Jan Stancek — 2013-05-16T20:25:19

Following commit converted some operations to use memcpy, but the size
of copied areas is now u_long, previously it was u_int32_t:
  commit fac3eb51c7055055bdd7dccd9db556d9e4a1cf35
  Author: Steve Dickson <steved-H+wXaHxf7aLQT0dZR+AlfA< at >public.gmane.org>
  Date:   Tue Feb 12 14:26:31 2013 -0500
      clnt_vc_control: Removed a strict-aliasing warning

This broke mount on ppc64/s390x:
  # ./mount.nfs -vvv -onfsvers=3,tcp rhel6-nfs:/export/home /mnt/test
  mount.nfs: timeout set for Fri May  3 15:20:14 2013
  mount.nfs: trying text-based options 'nfsvers=3,tcp,addr=10.1.1.5'
  mount.nfs: prog 100003, trying vers=3, prot=6
  mount.nfs: portmap query failed: RPC: Program/version mismatch
  mount.nfs: Protocol not supported

Signed-off-by: Jan Stancek <jstancek-H+wXaHxf7aLQT0dZR+AlfA< at >public.gmane.org>
---
 src/clnt_vc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/clnt_vc.c b/src/clnt_vc.c
index 7769809..2eab9e4 100644
--- a/src/clnt_vc.c
+++ b/src/clnt_vc.c
< at >< at > -505,7 +505,7 < at >< at > cln

Re: [PATCH 0/3] Speed up detection of whether or not rpc.gssd is running

J. Bruce Fields — 2013-05-16T20:22:29

I don't understand what you're referring to--could you elaborate?

--b.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA< at >public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] SUNRPC: Convert auth_gss pipe detection to work in namespaces

J. Bruce Fields — 2013-05-16T20:21:42

Might be worth deprecating the "legacy" version--add a warning in for
any users and then remove it if we don't see any evidence anyone's hit
it recently.

--b.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA< at >public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] SUNRPC: Faster detection if gssd is actually running

J. Bruce Fields — 2013-05-16T20:19:54

So it's still a 15 second delay on the first mount, then 7 on the
second, then 3, 1, and no delay thereafter.  Is that right?

Why not be harsher and go straight to 0 after the first failure?

--b.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA< at >public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/13] Security: Add Hook to test if the particular xattr is part of a MAC model.

Steve Dickson — 2013-05-16T15:56:37

From: David Quigley <dpquigl-a7DkhOHRHBuN9aS15agKxg< at >public.gmane.org>

The interface to request security labels from user space is the xattr
interface. When requesting the security label from an NFS server it is
important to make sure the requested xattr actually is a MAC label. This allows
us to make sure that we get the desired semantics from the attribute instead of
something else such as capabilities or a time based LSM.

Acked-by: James Morris <james.l.morris-QHcLZuEGTsvQT0dZR+AlfA< at >public.gmane.org>
Signed-off-by: Matthew N. Dodd <Matthew.Dodd-DABiIiYg7OfQT0dZR+AlfA< at >public.gmane.org>
Signed-off-by: Miguel Rodel Felipe <Rodel_FM-geVtEqcQUv4Eyxwt80+Gtti2O/JbrIOy< at >public.gmane.org>
Signed-off-by: Phua Eu Gene <PHUA_Eu_Gene-geVtEqcQUv4Eyxwt80+Gtti2O/JbrIOy< at >public.gmane.org>
Signed-off-by: Khin Mi Mi Aung <Mi_Mi_AUNG-geVtEqcQUv4Eyxwt80+Gtti2O/JbrIOy< at >public.gmane.org>
---
 include/linux/security.h   | 14 ++++++++++++++
 security/capability.c      |  6 ++++++
 security/security.c        |  6 ++++++
 security/se

Froe e71bf1d708e1294b3bae64d04f03228b3625f2a3 Mon Sep 17 00:00:00 2001

Steve Dickson — 2013-05-16T15:56:35

This is an updated to the linux-3-10.rc1 release. The minor 
changes in this release are:

* Added the needed Acked-by:

* Stop labels leaking into the cache_consistency_bitmask

David Quigley (7):
  Security: Add Hook to test if the particular xattr is part of a MAC
    model.
  LSM: Add flags field to security_sb_set_mnt_opts for in kernel mount
    data.
  SELinux: Add new labeling type native labels
  NFSv4: Add label recommended attribute and NFSv4 flags
  NFSv4: Introduce new label structure
  NFSv4: Extend fattr bitmaps to support all 3 words
  NFS: Extend NFS xattr handlers to accept the security namespace

Steve Dickson (6):
  Security: Add hook to calculate context based on a negative dentry.
  NFSv4.2: Added NFS v4.2 support to the NFS client
  NFS:Add labels to client function prototypes
  NFS: Add label lifecycle management
  NFS: Client implementation of Labeled-NFS
  Kconfig: Add Kconfig entry for Labeled NFS V4 client

 fs/nfs/Kconfig                      |  28 ++
 fs/nfs/callback.c

Re: [PATCH 0/3] Speed up detection of whether or not rpc.gssd is running

Chuck Lever — 2013-05-16T13:55:21

On May 15, 2013, at 3:50 PM, Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA< at >public.gmane.org> wrote:


Obviously my preference is to keep the existing krb5i -> sys logic and to try to address the upcall timeout, so this is a good direction, IMO.

After some review, I'm still a little concerned about initialization races inappropriately preventing upcalls, but time (and testing) will tell.  I'm happy with this solution if you and Bruce are.

Speaking of testing, I can test this series if you think that would be valuable, but I assume that those who originally reported the timeout problem (Jeff and Steve) should be the ones to confirm whether this series addresses their concern.