On Mon, Aug 10, 2015 at 10:43:46PM +0200, Martin Pieuchot wrote:
> In general these messages do not help. Here we have two cases of
> messages, either logged at LOG_INFO or LOG_ERR.
Yes, multiple logs for the same error are bad. But there is a path
where we ignore the return code of in6_update_ifa().
in6_update_ifa()
in6_ifattach_loopback()
in6_ifattach()
The latter is called without error check from
if_up()
ifioctl()
ifnewlladdr()
in6_control()
So we should either propagate the error in these functions or add
a log somewhere in the call stack.
bluhm
>
> In the first case a proper error code is returned via ioctl(2), so
> the userland application knows it has done something incorrect.
>
> The second case correspond to a ENOMEM error, which is something that
> can happen and is properly handled (goto cleanup).
>
> Ok?
>
> Index: netinet6/in6.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/in6.c,v
> retrieving revision 1.161
> diff -u -p -r1.161 in6.c
> --- netinet6/in6.c 18 Jul 2015 15:05:32 -0000 1.161
> +++ netinet6/in6.c 10 Aug 2015 20:38:04 -0000
> @@ -633,17 +633,10 @@ in6_update_ifa(struct ifnet *ifp, struct
> * must be 128.
> */
> if (ifra->ifra_dstaddr.sin6_family == AF_INET6) {
> - if ((ifp->if_flags & (IFF_POINTOPOINT|IFF_LOOPBACK)) == 0) {
> - /* XXX: noisy message */
> - nd6log((LOG_INFO, "in6_update_ifa: a destination can "
> - "be specified for a p2p or a loopback IF only\n"));
> + if ((ifp->if_flags & (IFF_POINTOPOINT|IFF_LOOPBACK)) == 0)
> return (EINVAL);
> - }
> - if (plen != 128) {
> - nd6log((LOG_INFO, "in6_update_ifa: prefixlen should "
> - "be 128 when dstaddr is specified\n"));
> + if (plen != 128)
> return (EINVAL);
> - }
> }
> /* lifetime consistency check */
> lt = &ifra->ifra_lifetime;
> @@ -703,10 +696,6 @@ in6_update_ifa(struct ifnet *ifp, struct
> */
> if (ia6->ia_prefixmask.sin6_len &&
> in6_mask2len(&ia6->ia_prefixmask.sin6_addr, NULL) != plen) {
> - nd6log((LOG_INFO, "in6_update_ifa: the prefix length of
> an"
> - " existing (%s) address should not be changed\n",
> - inet_ntop(AF_INET6, &ia6->ia_addr.sin6_addr,
> - addr, sizeof(addr))));
> error = EINVAL;
> goto unlink;
> }
> @@ -806,14 +795,8 @@ in6_update_ifa(struct ifnet *ifp, struct
> ifra->ifra_addr.sin6_addr.s6_addr32[3];
> llsol.sin6_addr.s6_addr8[12] = 0xff;
> imm = in6_joingroup(ifp, &llsol.sin6_addr, &error);
> - if (!imm) {
> - nd6log((LOG_ERR, "in6_update_ifa: "
> - "addmulti failed for %s on %s (errno=%d)\n",
> - inet_ntop(AF_INET6, &llsol.sin6_addr,
> - addr, sizeof(addr)),
> - ifp->if_xname, error));
> + if (!imm)
> goto cleanup;
> - }
> LIST_INSERT_HEAD(&ia6->ia6_memberships, imm, i6mm_chain);
>
> bzero(&mltmask, sizeof(mltmask));
> @@ -867,15 +850,8 @@ in6_update_ifa(struct ifnet *ifp, struct
> rtfree(rt);
> }
> imm = in6_joingroup(ifp, &mltaddr.sin6_addr, &error);
> - if (!imm) {
> - nd6log((LOG_WARNING,
> - "in6_update_ifa: addmulti failed for "
> - "%s on %s (errno=%d)\n",
> - inet_ntop(AF_INET6, &mltaddr.sin6_addr,
> - addr, sizeof(addr)),
> - ifp->if_xname, error));
> + if (!imm)
> goto cleanup;
> - }
> LIST_INSERT_HEAD(&ia6->ia6_memberships, imm, i6mm_chain);
>
> /*
> @@ -884,11 +860,6 @@ in6_update_ifa(struct ifnet *ifp, struct
> if (in6_nigroup(ifp, hostname, hostnamelen, &mltaddr) == 0) {
> imm = in6_joingroup(ifp, &mltaddr.sin6_addr, &error);
> if (!imm) {
> - nd6log((LOG_WARNING, "in6_update_ifa: "
> - "addmulti failed for %s on %s (errno=%d)\n",
> - inet_ntop(AF_INET6, &mltaddr.sin6_addr,
> - addr, sizeof(addr)),
> - ifp->if_xname, error));
> /* XXX not very fatal, go on... */
> } else {
> LIST_INSERT_HEAD(&ia6->ia6_memberships,
> @@ -935,14 +906,8 @@ in6_update_ifa(struct ifnet *ifp, struct
> rtfree(rt);
> }
> imm = in6_joingroup(ifp, &mltaddr.sin6_addr, &error);
> - if (!imm) {
> - nd6log((LOG_WARNING, "in6_update_ifa: "
> - "addmulti failed for %s on %s (errno=%d)\n",
> - inet_ntop(AF_INET6, &mltaddr.sin6_addr,
> - addr, sizeof(addr)),
> - ifp->if_xname, error));
> + if (!imm)
> goto cleanup;
> - }
> LIST_INSERT_HEAD(&ia6->ia6_memberships, imm, i6mm_chain);
> }
>
