Bug 441559

-------- 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

Created attachment 302005 [details]
draft of lockd part

Created attachment 302006 [details]
draft of statd part

Just about to start testing this - first bug: open return value 

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

Thanks Steve, will do. And no - patch does not work as such, possibly byte order
was wrong. Will fix now.

Bah. Fixing the fix indeed fixes it.

Created attachment 302152 [details]
tested nsm patch

Created attachment 302153 [details]
tested nlm patch

These patches seem a bit IPv4 specific.  Is there a way to do this in
more independent fashion?

You're right. We can certainly switch to using strings instead of addresses.

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?

Argh, sorry. So it will work; it will just expose everything out.

Two different patch variants that would address this issue have been sent to
linux-nfs list.

So the solution is to forget that -n existed and use -H with notify script, right?

Argh, so the solution:
- use -L to prevent statd from notifying anyone
- use sm-notify manually with -v to notify correct segment

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.

Created attachment 304538 [details]
Fixes -n lockd callbacks. Lockd will now see packets from lo.

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?

-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).

To clarify: -n is mandatory for rhel4 tree only as it does not have sm-notify.

Thanks a lot, that definitely helps.  

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

Right.

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.

I was able to reproduce the issue on local NokiaBlade cluster and verify that
patch indeed fixes it.

Since a patch has been developed and partner verified and this is an existing
RHEL 4 bug, requesting an exception for 4.7.

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?

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.

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.

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.

(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.

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.

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

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.

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.

Created attachment 309071 [details]
lockd callback fix for rhel5

Same fix for latest el5 nfs-utils.

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.

Patch is now also verified on customer site. Issue was preventing nodes from
booting.

fixed in nfs-utils-1.0.6-87.EL4

~ 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.

(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.

nfs-utils-1.0.6-87.EL4.i386.rpm is tested working on Westford Nokiablade cluster.

CustomerVerified comment #48

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

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.