Bug 441559 - lockd not aware of statd -n switch
lockd not aware of statd -n switch
Status: CLOSED ERRATA
Product: Red Hat Enterprise Linux 4
Classification: Red Hat
Component: nfs-utils (Show other bugs)
4.6
All Linux
medium Severity medium
: rc
: ---
Assigned To: Steve Dickson
: OtherQA
Depends On:
Blocks: 345201
  Show dependency treegraph
 
Reported: 2008-04-08 15:04 EDT by Janne Karhunen
Modified: 2013-08-05 20:43 EDT (History)
7 users (show)

See Also:
Fixed In Version: RHBA-2008-0742
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-07-24 16:00:22 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
draft of lockd part (2.79 KB, text/x-patch)
2008-04-10 11:01 EDT, Janne Karhunen
no flags Details
draft of statd part (1.54 KB, text/x-patch)
2008-04-10 11:02 EDT, Janne Karhunen
no flags Details
tested nsm patch (1.59 KB, patch)
2008-04-11 14:14 EDT, Janne Karhunen
no flags Details | Diff
tested nlm patch (3.01 KB, patch)
2008-04-11 14:14 EDT, Janne Karhunen
no flags Details | Diff
Fixes -n lockd callbacks. Lockd will now see packets from lo. (893 bytes, patch)
2008-05-05 12:52 EDT, Janne Karhunen
no flags Details | Diff
lockd callback fix for rhel5 (745 bytes, patch)
2008-06-12 10:12 EDT, Janne Karhunen
no flags Details | Diff

  None (edit)
Description Janne Karhunen 2008-04-08 15:04:05 EDT
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
Comment 1 Steve Dickson 2008-04-08 16:09:56 EDT
-------- Original Message --------
Subject: IT #137407
Date: Tue, 08 Apr 2008 13:49:43 -0400
From: Janne Karhunen <jkarhune@redhat.com>
To: steved@redhat.com

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
Comment 2 Janne Karhunen 2008-04-10 11:01:34 EDT
Created attachment 302005 [details]
draft of lockd part
Comment 3 Janne Karhunen 2008-04-10 11:02:03 EDT
Created attachment 302006 [details]
draft of statd part
Comment 4 Janne Karhunen 2008-04-10 13:34:45 EDT
Just about to start testing this - first bug: open return value 
Comment 5 Steve Dickson 2008-04-10 13:52:08 EDT
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
Comment 6 Janne Karhunen 2008-04-10 15:06:21 EDT
Thanks Steve, will do. And no - patch does not work as such, possibly byte order
was wrong. Will fix now.
Comment 7 Janne Karhunen 2008-04-11 12:36:40 EDT
Bah. Fixing the fix indeed fixes it.
Comment 8 Janne Karhunen 2008-04-11 14:14:15 EDT
Created attachment 302152 [details]
tested nsm patch
Comment 9 Janne Karhunen 2008-04-11 14:14:54 EDT
Created attachment 302153 [details]
tested nlm patch
Comment 10 Peter Staubach 2008-04-11 14:46:04 EDT
These patches seem a bit IPv4 specific.  Is there a way to do this in
more independent fashion?
Comment 11 Janne Karhunen 2008-04-11 15:04:37 EDT
You're right. We can certainly switch to using strings instead of addresses.
Comment 12 Janne Karhunen 2008-04-18 14:14:30 EDT
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?
Comment 13 Janne Karhunen 2008-04-18 14:42:39 EDT
Argh, sorry. So it will work; it will just expose everything out.
Comment 14 Janne Karhunen 2008-04-29 09:46:35 EDT
Two different patch variants that would address this issue have been sent to
linux-nfs list.
Comment 15 Janne Karhunen 2008-05-05 09:45:13 EDT
So the solution is to forget that -n existed and use -H with notify script, right?
Comment 16 Janne Karhunen 2008-05-05 11:30:18 EDT
Argh, so the solution:
- use -L to prevent statd from notifying anyone
- use sm-notify manually with -v to notify correct segment
Comment 17 Janne Karhunen 2008-05-05 12:23:17 EDT
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.
Comment 18 Janne Karhunen 2008-05-05 12:52:47 EDT
Created attachment 304538 [details]
Fixes -n lockd callbacks. Lockd will now see packets from lo.
Comment 19 James M. Leddy 2008-05-21 09:39:03 EDT
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?
Comment 20 Janne Karhunen 2008-05-21 09:52:58 EDT
-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).
Comment 21 Janne Karhunen 2008-05-21 10:18:14 EDT
To clarify: -n is mandatory for rhel4 tree only as it does not have sm-notify.
Comment 22 James M. Leddy 2008-05-21 17:46:13 EDT
Thanks a lot, that definitely helps.  

So, just to be sure, we are using
https://bugzilla.redhat.com/attachment.cgi?id=304538 , right?
Comment 23 Janne Karhunen 2008-05-22 09:36:08 EDT
Right.
Comment 24 RHEL Product and Program Management 2008-05-22 12:24:12 EDT
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.
Comment 26 Janne Karhunen 2008-06-10 13:38:05 EDT
I was able to reproduce the issue on local NokiaBlade cluster and verify that
patch indeed fixes it.
Comment 27 Russell Doty 2008-06-10 15:48:58 EDT
Since a patch has been developed and partner verified and this is an existing
RHEL 4 bug, requesting an exception for 4.7.
Comment 28 Peter Staubach 2008-06-10 16:04:44 EDT
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?
Comment 29 Russell Doty 2008-06-10 16:18:15 EDT
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.
Comment 30 Peter Staubach 2008-06-10 16:22:49 EDT
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.
Comment 31 Steve Dickson 2008-06-10 19:24:23 EDT
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.
Comment 32 Janne Karhunen 2008-06-10 19:56:28 EDT
(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.

Comment 34 Janne Karhunen 2008-06-11 10:18:53 EDT
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.
Comment 35 Janne Karhunen 2008-06-11 11:45:05 EDT
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
Comment 36 Janne Karhunen 2008-06-11 13:09:23 EDT
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.
Comment 37 Janne Karhunen 2008-06-11 14:25:59 EDT
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.
Comment 38 Janne Karhunen 2008-06-12 10:12:27 EDT
Created attachment 309071 [details]
lockd callback fix for rhel5

Same fix for latest el5 nfs-utils.
Comment 39 Peter Staubach 2008-06-12 11:26:07 EDT
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.
Comment 42 Janne Karhunen 2008-06-24 12:06:46 EDT
Patch is now also verified on customer site. Issue was preventing nodes from
booting.
Comment 44 Steve Dickson 2008-06-25 11:16:15 EDT
fixed in nfs-utils-1.0.6-87.EL4
Comment 46 Chris Ward 2008-06-27 07:37:42 EDT
~ Attention ~ Testing Feedback Deadlines are approaching!

This bug should fixed in Partner Snapshot 4, which is now available on
partners.redhat.com, ready for testing.

We are approaching the end of RHEL 4.7 testing cycle. It is important to let us
know of your testing results *as soon as possible*. If any issues are found once
the testing phase  deadline has passed, you might lose your opportunity to
include the fix in this RHEL update.

If you have verified that this bug has been properly fixed or have discovered
any issues, please provide detailed feedback of your testing results, including
the package version   and snapshot tested. The bug status will be updated for
you, based on testing results.

Contact your Partner Manager with any additional questions.
Comment 47 Patrick C. F. Ernzer 2008-06-27 09:57:29 EDT
(In reply to comment #46)
> ~ Attention ~ Testing Feedback Deadlines are approaching!
> 
> This bug should fixed in Partner Snapshot 4, which is now available on
> partners.redhat.com, ready for testing.

Snap 4 still has nfs-utils-1.0.6-86.EL4.i386.rpm and
  rpm -qpl --changelog nfs-utils-1.0.6-86.EL4.i386.rpm | grep 441559
gives no result

So I guess we have to wait until Snap 5 to test this.
Comment 48 Janne Karhunen 2008-06-27 13:35:50 EDT
nfs-utils-1.0.6-87.EL4.i386.rpm is tested working on Westford Nokiablade cluster.
Comment 49 Patrick C. F. Ernzer 2008-06-30 09:18:48 EDT
CustomerVerified comment #48
Comment 52 errata-xmlrpc 2008-07-24 16:00:22 EDT
An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHBA-2008-0742.html
Comment 54 Chris Ward 2008-07-29 03:31:09 EDT
Partners, I would like to thank you all for your participation in assuring the
quality of this RHEL 4.7 Update Release. My hat's off to you all. Thanks.

Note You need to log in before you can comment on or make changes to this bug.