Bug 451057

+++ This bug was initially created as a clone of Bug #441559 +++

Description of problem:

Lockd seems to reject callbacks from statd started with -n switch: it seems to
expect statd always to be bound to loopback address (RHEL4U6:
fs/lockd/svc4proc.c:429).


Version-Release number of selected component (if applicable):

nfs-utils-1.0.6-84 (probably valid for all..)


How reproducible:

Always

Steps to Reproduce:
1. Create NFS server fail over pair 
2. Migrate NFS server IP address between servers on failover
3. Try to bind server statd to migrated address on server startup using -n


Actual results:

Nov  8 13:27:55 warning CLA-0 kernel: lockd: rejected NSM callback from c0a80011:776
Nov  8 13:27:55 notice CLA-0 rpc.statd[31552]: recv_rply: [127.0.0.1] RPC status 5

Expected results:

No rejected callbacks

-- Additional comment from steved on 2008-04-08 16:09 EST --
-------- Original Message --------
Subject: IT #137407
Date: Tue, 08 Apr 2008 13:49:43 -0400
From: Janne Karhunen <jkarhune>
To: steved

Hi Steve,

I found a system that reproduces IT #137407. After having checked 
checked the address server lockd now whines about (rejected
statd callback from c0a80035), it turns out to be perfectly valid 
local server address (Directory). So, it should not reject it and
the call is valid.

Looking at:

static int
nlm4svc_proc_sm_notify(struct svc_rqst *rqstp, struct nlm_reboot *argp,
                                              void              *resp)
{
        struct sockaddr_in      saddr = rqstp->rq_addr;
        int                     vers = argp->vers;
        int                     prot = argp->proto >> 1;

        struct nlm_host         *host;

        dprintk("lockd: SM_NOTIFY     called\n");
        if (saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK)
         || ntohs(saddr.sin_port) >= 1024) {
                printk(KERN_WARNING
                        "lockd: rejected NSM callback from %08x:%d\n",
                        ntohl(rqstp->rq_addr.sin_addr.s_addr),
                        ntohs(rqstp->rq_addr.sin_port));
                return rpc_system_err;
        }
..

So lockd expects statd to be always bound to LOOPBACK. Now, 
looking at statd code we have:

int
statd_get_socket(int port)
{
        struct sockaddr_in      sin;

        if (sockfd >= 0)
                return sockfd;

        if ((sockfd = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP)) < 0) {
                note(N_CRIT, "Can't create socket: %m");
                return -1;
        }

        FD_SET(sockfd, &SVC_FDSET);

        memset(&sin, 0, sizeof(sin));
        sin.sin_family = AF_INET;
        sin.sin_addr.s_addr = INADDR_ANY;
        /*
         * If a local hostname is given (-n option to statd), bind to
the address
         * specified. This is required to support clients that ignore
the mon_name in
         * the statd protocol but use the source address from the
request packet.
         */
        if (MY_NAME) {
                struct hostent *hp = gethostbyname(MY_NAME);
                if (hp)
                        sin.sin_addr = *(struct in_addr *) hp->h_addr;
..


So these two are clearly out of sync. We are, indeed, using
-n Directory as statd manpage states that:

       -n, --name name
              specify  a  name  for  rpc.statd  to use as the local
hostname. By default, rpc.statd will call gethostname(2) to get the
              local hostname. Specifying a local hostname may be useful
for machines with more than one interfaces.
 
IMHO lockd should be aware of such case. Which is what we have
here.


-- 
// Janne


-- Additional comment from jkarhune on 2008-04-10 11:01 EST --
Created an attachment (id=302005)
draft of lockd part


-- Additional comment from jkarhune on 2008-04-10 11:02 EST --
Created an attachment (id=302006)
draft of statd part


-- Additional comment from jkarhune on 2008-04-10 13:34 EST --
Just about to start testing this - first bug: open return value 

-- Additional comment from steved on 2008-04-10 13:52 EST --
Janne,

When generating patch please the '-p' argument to the diff command 
so the functions names are added to the patch. This makes it
much easier to read the patch and find the changed code.

I used a modified version of /usr/bin/gendiff that has
the following change:

--- /usr/bin/gendiff.orig       2008-04-10 13:50:08.000000000 -0400
+++ /usr/bin/gendiff    2008-04-10 13:50:12.000000000 -0400
@@ -8,7 +8,7 @@
 
 find $1 \( -name "*$2" -o -name ".*$2" \) -print |
 while read f; do
-    U=-u
+    U=-up
     [ "`basename $f`" = "ChangeLog$2" ] && U=-U0
 #    diff ${U} $f `echo $f | sed s/$2\$//`
     if [ -r "$f" ]; then


-- Additional comment from jkarhune on 2008-04-10 15:06 EST --
Thanks Steve, will do. And no - patch does not work as such, possibly byte order
was wrong. Will fix now.

-- Additional comment from jkarhune on 2008-04-11 12:36 EST --
Bah. Fixing the fix indeed fixes it.

-- Additional comment from jkarhune on 2008-04-11 14:14 EST --
Created an attachment (id=302152)
tested nsm patch


-- Additional comment from jkarhune on 2008-04-11 14:14 EST --
Created an attachment (id=302153)
tested nlm patch


-- Additional comment from staubach on 2008-04-11 14:46 EST --
These patches seem a bit IPv4 specific.  Is there a way to do this in
more independent fashion?

-- Additional comment from jkarhune on 2008-04-11 15:04 EST --
You're right. We can certainly switch to using strings instead of addresses.

-- Additional comment from jkarhune on 2008-04-18 14:14 EST --
Humm; as it binds to INADDR_ANY without this option, is it actually completely
blind luck that it works at all? Or is this some obscure corner of the NLM protocol?

-- Additional comment from jkarhune on 2008-04-18 14:42 EST --
Argh, sorry. So it will work; it will just expose everything out.

-- Additional comment from jkarhune on 2008-04-29 09:46 EST --
Two different patch variants that would address this issue have been sent to
linux-nfs list.

-- Additional comment from jkarhune on 2008-05-05 09:45 EST --
So the solution is to forget that -n existed and use -H with notify script, right?

-- Additional comment from jkarhune on 2008-05-05 11:30 EST --
Argh, so the solution:
- use -L to prevent statd from notifying anyone
- use sm-notify manually with -v to notify correct segment

-- Additional comment from jkarhune on 2008-05-05 12:23 EST --
Very non-intrusive fix for RHEL family: remove the -n address bind from
statd_get_socket(). It should work straight out, all it doesn't do is the port
visibility limitation. Lockd will see packets from lo and -v is used for
sm-notify correctly.

-- Additional comment from jkarhune on 2008-05-05 12:52 EST --
Created an attachment (id=304538)
Fixes -n lockd callbacks. Lockd will now see packets from lo.


-- Additional comment from jleddy on 2008-05-21 09:39 EST --
What's the status of this?  There are 2 valid patches against rmtcall.c, one of
which appears to ignore -n altogether.  Is our official stance that -n is obsolete?

-- Additional comment from jkarhune on 2008-05-21 09:52 EST --
-n is absolutely mandatory for lock fail-over between server nodes that have
different names. Without -n server statd will send wrong mon_name to clients on
failover and hence clients would be completely confused who actually rebooted
(and would not initiate lock reclaim).

And no, that patch does not ignore -n. It only removes the -n address binding
that breaks lockd callbacks, so it relies on kernel mangling the packet source
address always to right outgoing network (that is, lo for lockd which makes the
callbacks work).


-- Additional comment from jkarhune on 2008-05-21 10:18 EST --
To clarify: -n is mandatory for rhel4 tree only as it does not have sm-notify.

-- Additional comment from jleddy on 2008-05-21 17:46 EST --
Thanks a lot, that definitely helps.  

So, just to be sure, we are using
https://bugzilla.redhat.com/attachment.cgi?id=304538 , right?

-- Additional comment from jkarhune on 2008-05-22 09:36 EST --
Right.

-- Additional comment from pm-rhel on 2008-05-22 12:24 EST --
This request was evaluated by Red Hat Product Management for inclusion in a Red
Hat Enterprise Linux maintenance release.  Product Management has requested
further review of this request by Red Hat Engineering, for potential
inclusion in a Red Hat Enterprise Linux Update release for currently deployed
products.  This request is not yet committed for inclusion in an Update
release.

-- Additional comment from jkarhune on 2008-06-10 13:38 EST --
I was able to reproduce the issue on local NokiaBlade cluster and verify that
patch indeed fixes it.

-- Additional comment from rdoty on 2008-06-10 15:48 EST --
Since a patch has been developed and partner verified and this is an existing
RHEL 4 bug, requesting an exception for 4.7.

-- Additional comment from staubach on 2008-06-10 16:04 EST --
What is the status of this patch with respect to upstream acceptance?

I had the impression that it wasn't well received.  Or am I confused?

-- Additional comment from rdoty on 2008-06-10 16:18 EST --
My understanding is that this problem doesn't exist in RHEL 5 - that the
upstream changes since RHEL 4 eliminate the need for this patch.

-- Additional comment from staubach on 2008-06-10 16:22 EST --
I would think that the RHEL-4 solution should mirror that for upstream
and/or RHEL-5 as closely as possible.  I am not sure that using one
solution in RHEL-4 and after upgrading to RHEL-5, using a different
solution, makes much sense to me.

-- Additional comment from steved on 2008-06-10 19:24 EST --
Well the RHEL-5 nfs-utils appears to have the same problem as the
RHEL-4 on since they basically do the same thing. 

Now quickly looked into back porting the upstream solution which means 
introducing the sm-notify binary. The first patch what would be needed is 
a 742 line patch. Unfortunately this would not be the only patch needed. 

So I am of the option that if Janne's patch (which is much smaller, 5 lines
and not as risky) works we should go with it... 

But do think we should wait for Janne to complete all of his testing.
Then at the point, decide if the patch should be committed 
based on his results.


-- Additional comment from jkarhune on 2008-06-10 19:56 EST --
(In reply to comment #28)
> What is the status of this patch with respect to upstream acceptance?
> 
> I had the impression that it wasn't well received.  Or am I confused?

Yes you are. It's already in, and has been since 1.1.



-- Additional comment from jkarhune on 2008-06-11 10:18 EST --
FYI: reason this randomly worked for us is probably that bind() itself is
somewhat broken. In RHEL4 you can seemingly bind an interface that does not
exist, but in such case you get ANY regardless of what you asked. When you get
ANY it works right.

-- Additional comment from jkarhune on 2008-06-11 11:45 EST --
Just to make sure everyone understands the only downside of this patch. 

Here we have a packet capture when NFS server is failing over from CLA-0 to
CLA-1, with patch. This is seen from client side.

Directory (NFS) address: 192.168.0.64
CLA-1: 192.168.0.5

tshark -V -R stat -i bond0

    Source: 192.168.0.5 (192.168.0.5)
    Destination: 192.168.0.7 (192.168.0.7)

Network Status Monitor Protocol
    [Program Version: 1]
    [V1 Procedure: NOTIFY (6)]
    Status Change
        Monitor ID Name: Directory
            length: 9
            contents: Directory
            fill bytes: opaque data
    State: 683

Protocol mandates you to use 'nonitor name' (-n) to detect who rebooted. That
information is correct in this patched case. 

Client that would use packet source address as an server name would not work
correctly if kernel gets the source wrong, as is the case above. But:
1) client is doing something wrong if it prefers SOURCE to NAME
2) theres zero chance that this has ever worked, except in the case where bind
does the ANY bind anyway.

As to get this absolutely right for RHEL5 (as in not leaving any chance for the
client to misinterpret things) we would need to:
1) make sure bind works reliably for given interface X and does not succeed on
non-existent interfaces
2) backport the whole sm-notify concept if its not already there


-- Additional comment from jkarhune on 2008-06-11 13:09 EST --
Tested following cases:
1) complete cluster restart(s)
2) node restart(s)
3) server failover(s)
4) node restarts after server failover

Also did a protocol traces and only found the inconsistency explained above.
There were no rejected callbacks, all mon_names were set correctly and even
source addresses were correct with the above mentioned exception.

-- Additional comment from jkarhune on 2008-06-11 14:25 EST --
I'm betting mainline needs a patch here as well. sm-notify should have
IP_FREEBIND set to zero (setsockopt) to make sure source inconsistency does not
happen there as well. This would prevent bind from succeeding when in fact, it
did nothing.

-- Additional comment from jkarhune on 2008-06-12 10:12 EST --
Created an attachment (id=309071)
lockd callback fix for rhel5

Same fix for latest el5 nfs-utils.

-- Additional comment from staubach on 2008-06-12 11:26 EST --
You will probably need to clone this bug for RHEL-5 and then attach
the RHEL-5 patch to it.  Usually, 1 bugzilla for fix per release.