netdev - Re: [PATCH v2 net-next 1/3] rds: Changing IP address internal representation to struct in6

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <116715eb-12f0-94d9-279c-b06b89d7dac7@oracle.com>
Date:   Fri, 6 Jul 2018 07:55:18 -0700
From:   "santosh.shilimkar@...cle.com" <santosh.shilimkar@...cle.com>
To:     Ka-Cheong Poon <ka-cheong.poon@...cle.com>, netdev@...r.kernel.org
Cc:     davem@...emloft.net, rds-devel@....oracle.com
Subject: Re: [PATCH v2 net-next 1/3] rds: Changing IP address internal
 representation to struct in6_addr

On 7/6/18 2:08 AM, Ka-Cheong Poon wrote:
> On 07/06/2018 01:58 AM, Santosh Shilimkar wrote:
> 

> 
>>> diff --git a/net/rds/connection.c b/net/rds/connection.c
>>> index abef75d..ca72563 100644
>>> --- a/net/rds/connection.c
>>> +++ b/net/rds/connection.c
>>
>>> @@ -142,9 +151,12 @@ static void __rds_conn_path_init(struct 
>>> rds_connection *conn,
>>>    * are torn down as the module is removed, if ever.
>>>    */
>>>   static struct rds_connection *__rds_conn_create(struct net *net,
>>> -                        __be32 laddr, __be32 faddr,
>>> -                       struct rds_transport *trans, gfp_t gfp,
>>> -                       int is_outgoing)
>>> +                        const struct in6_addr *laddr,
>>> +                        const struct in6_addr *faddr,
>>> +                        struct rds_transport *trans,
>>> +                        gfp_t gfp,
>>> +                        int is_outgoing,
>>> +                        int dev_if)
>> Am just wondering if we can handle local link address case differently
>> instead of pushing the interface index everywhere. Did you think about
>> any alternative. This can also avoid bunch of changes again and hence
>> the question. Am trying to see if we can minimize the changes to
>> absolute must have to support IPv6.
> 
> 
> If link local address is supported, scoped ID must be used.
> Unless we remove the support of link local address, we need
> to keep scope ID.
>
Ok.

> 
>>> diff --git a/net/rds/ib.h b/net/rds/ib.h
>>> index a6f4d7d..12f96b3 100644
>>> --- a/net/rds/ib.h
>>> +++ b/net/rds/ib.h
> 
>>> +union rds_ib_conn_priv {
>>> +    struct rds_ib_connect_private    ricp_v4;
>>> +    struct rds6_ib_connect_private    ricp_v6;
>>>   };
>> This change was invetiable. Just add a comment saying the priviate
>> data size for v6 vs v4 is  different. Also for IPv6 priviate data,
>> I suggest add some resrve fields for any future extensions so
>> that things can be added without breaking wire protocol. With IPv4,
>> we ended up in bit of mess.
> 
> 
> Will add some comments.  Unfortunately the IB private data
> exchange has only a limited space.  I don't think there is
> any more space left for reserved field.  I think we should
> think of a different way to handle extensions in future.
>
There is enough space. You can send upto 512 bytes iirc. Please
add some reserve fields and ping me if you run into issues.

>>> diff --git a/net/rds/send.c b/net/rds/send.c
>>> index 94c7f74..6ed2e92 100644
>>> --- a/net/rds/send.c
>>> +++ b/net/rds/send.c
> 
>>> @@ -1081,27 +1085,59 @@ int rds_sendmsg(struct socket *sock, struct 
>>> msghdr *msg, size_t payload_len)
>>>           goto out;
>>>       }
>>> -    if (msg->msg_namelen) {
>>> -        /* XXX fail non-unicast destination IPs? */
>>> -        if (msg->msg_namelen < sizeof(*usin) || usin->sin_family != 
>>> AF_INET) {
>>> +    namelen = msg->msg_namelen;
>>> +    if (namelen != 0) {
>>> +        if (namelen < sizeof(*usin)) {
>>> +            ret = -EINVAL;
>>> +            goto out;
>>> +        }
>>> +        switch (namelen) {
>>> +        case sizeof(*usin):
>>> +            if (usin->sin_family != AF_INET ||
>>> +                usin->sin_addr.s_addr == htonl(INADDR_ANY) ||
>>> +                usin->sin_addr.s_addr == htonl(INADDR_BROADCAST) ||
>>> +                IN_MULTICAST(ntohl(usin->sin_addr.s_addr))) {
>>> +                ret = -EINVAL;
>> The idea was to fail non-unicast IP someday but can you not add this
>> change as part of v6 series. We can look at it later unless you need
>> this change for v6. Again the question is mainly to add only necessary
>> check and leave the existing behavior as is so please re-check all below
>> checks too.
> 
> 
> This is to match the same IPv6 check.  In IPv6 code, it checks
> if the address is a unicast address.  I can remove the IPv4
> checks but if an app does send to a multicast address, the
> connection will be stuck forever.
>
OK. Lets keep it then.

> 
>>> diff --git a/net/rds/transport.c b/net/rds/transport.c
>>> index 0b188dd..c9788db 100644
>>> --- a/net/rds/transport.c
>>> +++ b/net/rds/transport.c
>>
>>> +    if (ipv6_addr_v4mapped(addr)) {
>>
>> Dave already commented on ipv6_addr_v4mapped(). Apart from
>> some of those comments questions, rest of the patch looks
>> really good and nicely done. Will also look at your
>> subsequent two patches and try to send you comments in next
>> few days.
> 
> 
> Do you mean using mapped address to create IPv4 connections?
> I already have it in my work space.  Will be in v3.

yeah. Thanks !!