netdev - Re: [IPv6] Update setsockopt(IPV6_MULTICAST

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, 10 Oct 2007 14:48:38 -0700
From:	David Stevens <dlstevens@...ibm.com>
To:	Brian Haley <brian.haley@...com>
Cc:	David Miller <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	netdev-owner@...r.kernel.org,
	YOSHIFUJI Hideaki <yoshfuji@...ux-ipv6.org>
Subject: Re: [IPv6] Update setsockopt(IPV6_MULTICAST_IF) to support RFC 3493

Brian Haley <brian.haley@...com> wrote on 10/10/2007 02:20:45 PM:

> David Stevens wrote:
> > What about just checking for 0 in the later test?
> > 
> >         if (val && __dev_get_by_index(val) == NULL) {
> 
> We could fail the next check right before that though:

        Right, the semantics there would be "if we have a bound
dev if, that's the only legal value here." Setting it to '0' in
that case doesn't really do anythng, anyway. But I don't care
about that semantic difference-- could even add "val &&" to the
bound_dev_if check.
        What I don't like is that your "if" creates an identical
duplicate code path for the functional part of it. In this case
it's trivial (the asignment), but makes the code look more
complex than it really is. If v4 does it that way, I don't
like that either. :-)
        I agree with it in general, and may not be worth the
trouble, but I'd personally prefer something like:

        if (sk->sk_type == SOCK_STREAM)
                goto e_inval;
        if (val && sk->sk_bound_dev_if && sk->sk_bound_dev_if != val)
                goto e_inval;

        if (val && __dev_get_by_index(val) != NULL) {
                retv = -ENODEV;
                break;
        }
[at this point all validity checks are done and we're following
        one code path to do the work; each check is easily
        identifiable.]

        np->mcast_oif = val;
        retv = 0;
        break;

Or maybe:

        if (sk->sk_type == SOCK_STREAM)
                goto e_inval;

        if (val) {
                if (sk->sk_bound_dev_if && sk->sk_bound_dev_if != val)
                        goto e_inval;
                if (__dev_get_by_index(val != NULL) {
                        retv = -ENODEV;
                        break;
                }
        }
        np->mcast_oif = val;
        retv = 0;
        break;

But anyway, I made the comment; I think some form of it
should go in. :-) If you like the original better, that's
ok with me, too.

                                                +-DLS

-
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