netdev - Re: [PATCH net-next v3 1/3] net: ipv6: Unduplicate {raw,udp}v6

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]

Date:	Wed, 23 Apr 2014 15:37:13 +0900
From:	Lorenzo Colitti <lorenzo@...gle.com>
To:	Lorenzo Colitti <lorenzo@...gle.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	YOSHIFUJI Hideaki <yoshfuji@...ux-ipv6.org>,
	David Miller <davem@...emloft.net>,
	Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCH net-next v3 1/3] net: ipv6: Unduplicate
 {raw,udp}v6_sendmsg code

On Wed, Apr 23, 2014 at 12:48 AM, Hannes Frederic Sowa
<hannes@...essinduktion.org> wrote:
>> +     *optp = NULL;
>> +     *dstp = NULL;
>> +     *hlimit = *tclass = *dontfrag = -1;
>
> Do we need those? If we return != 0 from ip6_datagram_send_common we
> know the in/out arguments may or maybe got an update an as such cannot
> use them.
>
> Caller can initialize dstp = NULL so we can pass dstp to dst_release in the
> error path in the caller function (dst_release checks for argument != NULL).

Initalizing optp was not useful. As you say, the callers won't use it
if we return an error. I've removed it.

I've also removed the initialization of dstp. After thinking about it,
I agree it's bad for the callers to rely on this function to set dstp
to NULL on failure. It's more reasonable for the callers to assume
that the dst pointer they pass in won't be touched on failure.

As for hlimit, tclass and dontfrag, they (and connected), I think it's
useful to initialize them here because it simplifies the code. Since
we set them to -1 early on, we can later do things like "if (*hlimit <
0) { set the default hop limit }" and have things work regardless of
whether a hop limit was passed in via cmsg. I don't think it's
confusing: variables are all calculated and returned by this function,
and the callers should not be setting them since they have no
information on what to set them to. Also, they don't need to be freed.
I've moved them closer to where they are actually set.

>> +     if (!ipv6_addr_any(daddr))
>> +             fl6->daddr = *daddr;
>> +     else
>> +             fl6->daddr.s6_addr[15] = 0x1; /* :: means loopback (BSD'ism) */
>
> Maybe a newline?

Added.

> We would also need a fl6_sock_release here because we finished processing
> it.
>
> You could also release the flowlabel after fl6_merge_options and could remove
> the fl6_sock_release after ip6_sk_dst_lookup_flow in the error handling.

Yes, that's better. Done.
--
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