netdev - Re: [PATCH net-next 04/11] udp: Handle VRF device in sendmsg

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-next>] [day] [month] [year] [list]

Message-ID: <CALx6S36MAQRgEqcJbTO_m6eMEAzoJ6k3o67z7EgLa3BjUg6MbQ@mail.gmail.com>
Date:	Fri, 14 Aug 2015 20:16:16 -0700
From:	Tom Herbert <tom@...bertland.com>
To:	Shrijeet Mukherjee <shm@...ulusnetworks.com>
Cc:	David Ahern <dsa@...ulusnetworks.com>,
	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	Roopa Prabhu <roopa@...ulusnetworks.com>,
	Andy Gospodarek <gospo@...ulusnetworks.com>,
	Jon Toppins <jtoppins@...ulusnetworks.com>,
	Nikolay Aleksandrov <nikolay@...ulusnetworks.com>,
	Dinesh Dutt <ddutt@...ulusnetworks.com>,
	Hannes Frederic Sowa <hannes@...essinduktion.org>,
	Nicolas Dichtel <nicolas.dichtel@...nd.com>,
	Stephen Hemminger <stephen@...workplumber.org>,
	Jamal Hadi Salim <hadi@...atatu.com>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	"David S. Miller" <davem@...emloft.net>, svaidya@...cade.com
Subject: Re: [PATCH net-next 04/11] udp: Handle VRF device in sendmsg

On Fri, Aug 14, 2015 at 10:58 AM, Shrijeet Mukherjee
<shm@...ulusnetworks.com> wrote:
>
>
> On Fri, Aug 14, 2015 at 9:27 AM, Tom Herbert <tom@...bertland.com> wrote:
>>
>> On Thu, Aug 13, 2015 at 1:59 PM, David Ahern <dsa@...ulusnetworks.com>
>> wrote:
>> > For unconnected UDP sockets using a VRF device lookup source address
>> > based on VRF table. This allows the UDP header to be properly setup
>> > before showing up at the VRF device via the dst.
>> >
>> > Signed-off-by: Shrijeet Mukherjee <shm@...ulusnetworks.com>
>> > Signed-off-by: David Ahern <dsa@...ulusnetworks.com>
>> > ---
>> >  net/ipv4/udp.c | 22 +++++++++++++++++++++-
>> >  1 file changed, 21 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
>> > index 83aa604f9273..7af5052e3b1f 100644
>> > --- a/net/ipv4/udp.c
>> > +++ b/net/ipv4/udp.c
>> > @@ -1013,11 +1013,31 @@ int udp_sendmsg(struct sock *sk, struct msghdr
>> > *msg, size_t len)
>> >
>> >         if (!rt) {
>> >                 struct net *net = sock_net(sk);
>> > +               __u8 flow_flags = inet_sk_flowi_flags(sk);
>> >
>> >                 fl4 = &fl4_stack;
>> > +
>> > +               /* unconnected socket. If output device is enslaved to a
>> > VRF
>> > +                * device lookup source address from VRF table. This
>> > mimics
>> > +                * behavior of ip_route_connect{_init}.
>> > +                */
>> > +               if (netif_index_is_vrf(net, ipc.oif)) {
>> > +                       flowi4_init_output(fl4, ipc.oif, sk->sk_mark,
>> > tos,
>> > +                                          RT_SCOPE_UNIVERSE,
>> > sk->sk_protocol,
>> > +                                          (flow_flags |
>> > FLOWI_FLAG_VRFSRC),
>> > +                                          faddr, saddr, dport,
>> > +                                          inet->inet_sport);
>> > +
>> > +                       rt = ip_route_output_flow(net, fl4, sk);
>> > +                       if (!IS_ERR(rt)) {
>> > +                               saddr = fl4->saddr;
>> > +                               ip_rt_put(rt);
>> > +                       }
>> > +               }
>> > +
>>
>> I really don't like this. It seems like you're putting device specific
>> code in a critical L4 data path function. Also, does ipv6/udp.c need
>> be updated similarly? Why can't VRF be abstracted out in routing
>> lookups?
>
>
> Tom,
>
> Did not have a better way to make this work. The point of the VRF driver was
> to be completely transparent for anything other routing lookups. Modifying
> the header in the driver means that fragmentation etc will have trouble.
>
> So this code really just makes the saddr evaluation before we enter the udp
> code path and is similar to what the tcp side does. If you have a suggestion
> on a different and hopefully consistent way to do with tcp and ipv6, that
> would be preferable.

At least collect this code into one (static inline) function to better
minimize the code churn in udp. If this is general functionality that
can be used by other drivers then abstract it out as such. Also, if
the VRF driver is not configured it seems like this code should
compiled out. As it stands now "if (netif_index_is_vrf(net, ipc.oif))
{" adds a conditional to every call of udp_sendmsg rather or not we
are using VRF :-(.

Tom
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html