> On May 25, 2018, at 10:04 AM, Chuck Lever <chuck.lever@xxxxxxxxxx> wrote: > > > >> On May 25, 2018, at 9:44 AM, Olga Kornievskaia <aglo@xxxxxxxxx> wrote: >> >> On Fri, May 25, 2018 at 12:24 PM, Chuck Lever <chuck.lever@xxxxxxxxxx> wrote: >>> Hi Olga- >>> >>>> On May 25, 2018, at 7:02 AM, Olga Kornievskaia <aglo@xxxxxxxxx> wrote: >>>> >>>> Thank you for the comments. Will hopefully address them in the next version. >>>> >>>> On Thu, May 24, 2018 at 8:50 PM, Chuck Lever <chuck.lever@xxxxxxxxxx> wrote: >>>>> Hi Olga- >>>>> >>>>>> On May 24, 2018, at 1:05 PM, Olga Kornievskaia <kolga@xxxxxxxxxx> wrote: >>>>>> >>>>>> If the user supplies a clientaddr value, >>>>> >>>>> Please say "NFS client administrator" not "user". A >>>>> "user" is any non-privileged user, so saying that a >>>>> "user" can set this value is misleading. >>>> >>>> Ok will change it. >>>> >>>>>> it should be either >>>>>> a special value of either IPV4/IPV6 any address or a local address >>>>>> on the same network that the server being mounted. >>>>> >>>>> This option should allow any local address the client has, >>>>> not just an address that is on the same network as the >>>>> server. See below for further explanation. >>>> >>>> Ok, I added this to the comment specifically as I didn't know if this >>>> would pose a problem. I didn't know if allowing any address was useful >>>> as when it's not specified the address on the same network as the >>>> server is chosen. >>> >>> Yep, any of the client's local addresses should be allowed. >>> >>> >>>>>> Otherwise, we >>>>>> disallow the client to use an arbitrary value of the clientaddr value. >>>>>> This value is used to construct a client id of SETCLIENTID and >>>>>> providing a false value can interfere with the real owner's mount. >>>>> >>>>> The patch description is misleading: >>>>> >>>>> Interference occurs only if the real owner's lease is >>>>> not protected by Kerberos AND this client has the same >>>>> client ID string as another client. >>>> >>>> Ok I will add this more explicit detail when the interference occurs >>>> (when neither of the machines are using Kerberos and the other client >>>> machine is not using a module parameter to add a unique identifier to >>>> the client ID string). I think otherwise it is knowns that client ID >>>> is created with the value of the clientaddr. >>> >>> The only way a problem occurs is if the clientaddr is the >>> same AND the cl_nodename is the same. How is that happening? >> >> Client ID in the SETCLIENTID is constructed by >> nfs4_init_nonuniform_client_string() function and it uses cl_ipaddr >> and not cl_nodename. > > 5614 scnprintf(str, len, "Linux NFSv4.0 %s/%s %s", > 5615 clp->cl_ipaddr, > 5616 rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_ADDR), > 5617 rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_PROTO)); > > So I get your point now: if two Linux NFSv4.0 clients mount the > same server and specify the same callback address, and do not > specify "migration", they will indeed present exactly the same > client ID string to the server. > > I had remembered there was more in that string, but I guess I > was thinking of the uniform client ID string. > > And it will break badly if several clients decide to use > clientaddr=0.0.0.0. Trond, any thoughts? Why aren't the > nodename or uniquifier available in this string? I can think of legitimate cases where two unique NFSv4.0 clients having the same IP address might mount the same server. - clientaddr=0.0.0.0, as mentioned above - clients behind NATs using a private subnet that happen to be numbered the same (192.168.0.77, say). Restricting the addresses allowed as the clientaddr= value does not address these cases. We already have some workarounds: - Use NFSv4.1 or later - Use NFSv4.0 with the "migration" option - Use Kerberos (give the clients and server service principals) and fix the server to reject SETCLIENTID using a recognized client ID string but an unrecognized authentication flavor and principal And possible fixes might include: - Improve the Linux client's non-UCS format - Make "migration" the default behavior I am still in favor of validating the clientaddr value, but again, IMO that just papers over the real problem, which is the current non-UCS client ID string format. Have a restful holiday weekend. >>> Why are the cl_nodenames the same? If they are not the same, >>> then it is not possible that the clients' leases are inter- >>> fering with each other, and something else is going on. >>> >>> >>>>> The Linux client's client ID string also contains the >>>>> system's cl_nodename. Both the cl_nodename and the >>>>> callback address have to be the same as some other >>>>> client's, and they both have to be Linux, for this to >>>>> be a problem. >>>>> >>>>> It's more likely that the customer's clients are all >>>>> named the same (maybe they are copied from the same >>>>> system image), and reverse DNS lookup is giving them >>>>> all the same clientaddr= . That's an unsupported >>>>> configuration and there are already ways to address >>>>> this. >>>>> >>>>> Or perhaps I don't understand the use case that is >>>>> causing the problem. Can the patch description explain >>>>> why your customer is trying to set clientaddr= ? >>>> >>>> The customer case was a simple mistake of including the wrong address. >>> >>> But that doesn't answer the question. Why did the >>> customer feel the need to set clientaddr= ? >> >> I don't know. In the end they decided they didn't need the clientaddr at all. >> >>>> Do you fundamentally disagree that there is a case for >>>> denial-of-service here? >>> >>> The only service that is affected if the clientaddr is >>> set incorrectly is on the client where the mistake >>> occurs. If the cl_nodenames are all unique then other >>> clients should not be affected by the mistake. If >>> that is happening, that's a server bug. >>> >>> If the problem was that the customer set the wrong >>> address, let's say that, rather than claiming that the >>> patch prevents lease tampering. >> >> Ok I can change it to lease tampering (I really don't care that much). >> But to just to discuss a bit further, how's lease tampering not a >> denial-of-service? It interfere with a client's ability to make >> progress. > > > -- > Chuck Lever > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Chuck Lever -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
