Skip to contentSkip to conversation

RFC: nlmclnt_grant() address check vs. callback address binding

PATCH Frank van Maarseveen 3 participants 7 messages Jul 2, 2007 – Jul 3, 2007

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

Frank van Maarseveen

author
#0 lore raw

Occasionally an NFS server needs to create an RPC transport for client callbacks, e.g. when granting a blocked lock request. When a server has multiple IP addresses (multihomed/multiple NICs or on a single interface) then the client may be called back from a different address because the server is free to select one: xs_udp_connect_worker() and xs_tcp_connect_worker() call xs_bindresvport() only when a reserved port is requested and xs_bindresvport() implicitly uses INADDR_ANY.

Currently this breaks locking when the server has more than one IP address reachable from the client because nlmclnt_grant() on the client rejects GRANT_MSG when the source IP address doesn't match the destination address in the original request. This is happening in a real-world situation where NFS servers have been virtualized using different IP addresses on a single machine.

Basically there are two ways to fix this:
1. server callbacks should bind to a local address, not INADDR_ANY.
2. clients should accept callbacks from a different server address.

At first I thought fixing the server is the right thing to do because the server behavior is arguably incorrect. Not easy but certainly doable (I'd do it if it has a chance of being accepted). It might also help NATting NFS traffic in some cases. However:

- I'm not sure if an address check such as in nlmclnt_grant()
is actually buying us anything.
- clients requiring a correct callback src address might not
work with other NFS server implementations and/or (future?) concepts for clustering/load balancing.
- Multiple IP addresses may be a workaround for reserved port
shortage.

As it currently stands, mounting from a secondary server IP address is broken w.r.t. locking when both client and server run linux.

Any opinion about (1) and (2)? maybe we should do both?

Currently I use (2) as a workaround:

fs/lockd/clntlock.c +9 -2
@@ -124,8 +124,15 @@ __be32 nlmclnt_grant(const struct sockad
 		 */
 		if (fl_blocked->fl_u.nfs_fl.owner->pid != lock->svid)
 			continue;
-		if (!nlm_cmp_addr(&block->b_host->h_addr, addr))
-			continue;
+		/*
+		 * The granted message may come from a different IP address
+		 * when the server has more than one. Warn when debug is on.
+		 */
+		if (!nlm_cmp_addr(&block->b_host->h_addr, addr)) {
+			dprintk("nlmclnt_grant: "NIPQUAD_FMT" != "NIPQUAD_FMT"\n",
+				NIPQUAD(block->b_host->h_addr.sin_addr.s_addr),
+				NIPQUAD(addr->sin_addr.s_addr));
+		}
 		if (nfs_compare_fh(NFS_FH(fl_blocked->fl_file->f_path.dentry->d_inode) ,fh) != 0)
 			continue;
 		/* Alright, we found a lock. Set the return status
Signature · 10 lines
Frank

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
#1 lore raw
Quoted 7 lines · Frank van Maarseveen

At first I thought fixing the server is the right thing to do because the server behavior is arguably incorrect. Not easy but certainly doable (I'd do it if it has a chance of being accepted). It might also help NATting NFS traffic in some cases. However:

(quoted diff) +0 -1
-	I'm not sure if an address check such as in nlmclnt_grant()

is actually buying us anything.

Exactly how else is the client going to figure out which lock this corresponds to?

Quoted 3 lines
(quoted diff) +0 -1
-	clients requiring a correct callback src address might not

work with other NFS server implementations and/or (future?) concepts for clustering/load balancing.

NFSv3 development is dead. NLM locking development is doubly so...

Quoted 2 lines
(quoted diff) +0 -1
-	Multiple IP addresses may be a workaround for reserved port

shortage.

There is _NO_ way for the client to figure out what lock the granted request refers to if the server starts transmitting from random IP addresses.
* There is no universal namespace for filehandles that can be used to identify which file the granted request is meant for
* The cookie is not guaranteed to be the same as that sent by the client for the LOCK request
* There is nothing else in the nlm_lock that can be used to identify which file we're talking about.

Trond

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

author
#2 lore raw
Quoted 7 lines · Trond Myklebust
Quoted 5 lines · Frank van Maarseveen

At first I thought fixing the server is the right thing to do because the server behavior is arguably incorrect. Not easy but certainly doable (I'd do it if it has a chance of being accepted). It might also help NATting NFS traffic in some cases. However:

Quoted 5 lines
Quoted 2 lines
(quoted diff) +0 -1
-	I'm not sure if an address check such as in nlmclnt_grant()

is actually buying us anything.

Exactly how else is the client going to figure out which lock this corresponds to?

The filehandle + pid/svid + lock start + lock end as it is doing right now. Filehandles offer a pretty strong check already because they are designed to handle inode reuse too.

The 32 bit pid/svid/lockowner number is conclusive.

Quoted 6 lines
Quoted 4 lines
(quoted diff) +0 -1
-	clients requiring a correct callback src address might not

work with other NFS server implementations and/or (future?) concepts for clustering/load balancing.

NFSv3 development is dead. NLM locking development is doubly so...

Not everyone is using (or can use) NFSv4. This is not about developing NFSv3 but about optimizing/fixing it in its current deployment.

Quoted 13 lines
Quoted 3 lines
(quoted diff) +0 -1
-	Multiple IP addresses may be a workaround for reserved port

shortage.

There is _NO_ way for the client to figure out what lock the granted request refers to if the server starts transmitting from random IP addresses.
* There is no universal namespace for filehandles that can be used to identify which file the granted request is meant for
* The cookie is not guaranteed to be the same as that sent by the client for the LOCK request
* There is nothing else in the nlm_lock that can be used to identify which file we're talking about.

The svid/pid. See nlm_find_lockowner().

Signature · 10 lines
Frank

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
#3 lore raw
Quoted 11 lines · Frank van Maarseveen
Quoted 9 lines

There is _NO_ way for the client to figure out what lock the granted request refers to if the server starts transmitting from random IP addresses.
* There is no universal namespace for filehandles that can be used to identify which file the granted request is meant for
* The cookie is not guaranteed to be the same as that sent by the client for the LOCK request
* There is nothing else in the nlm_lock that can be used to identify which file we're talking about.

The svid/pid. See nlm_find_lockowner().

nlm_find_lockowner() allocates a svid/pid that is unique only on a per-server basis.

Trond

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

author
#4 lore raw
Quoted 12 lines · Trond Myklebust
Quoted 10 lines · Frank van Maarseveen
Quoted 9 lines

There is _NO_ way for the client to figure out what lock the granted request refers to if the server starts transmitting from random IP addresses.
* There is no universal namespace for filehandles that can be used to identify which file the granted request is meant for
* The cookie is not guaranteed to be the same as that sent by the client for the LOCK request
* There is nothing else in the nlm_lock that can be used to identify which file we're talking about.

Quoted 4 lines
Quoted 1 line

The svid/pid. See nlm_find_lockowner().

nlm_find_lockowner() allocates a svid/pid that is unique only on a per-server basis.

Reading code... hmm, yes. One global 32 bit pid instead of one per server would remove that problem. Note that this is still a very theoretical problem because of all the other checks (filehandle check being the most important one probably).

Signature · 10 lines
Frank

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
#5 lore raw

On Tue, 2007-07-03 at 22:47 +0200, Frank van Maarseveen wrote

Quoted 4 lines

Reading code... hmm, yes. One global 32 bit pid instead of one per server would remove that problem. Note that this is still a very theoretical problem because of all the other checks (filehandle check being the most important one probably).

Fix the server instead... There is absolutely no reason to be accepting GRANTED calls from random IP addresses.

Trond

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

#6 lore raw
In reply to Frank van Maarseveen
Quoted 3 lines · Frank van Maarseveen

At first I thought fixing the server is the right thing to do because the server behavior is arguably incorrect. Not easy but certainly doable (I'd do it if it has a chance of being accepted).

Makes sense to me.

--b.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs