On Tue, Nov 10, 2015 at 6:12 PM, Daniel Borkmann <dan...@iogearbox.net> wrote: > On 11/10/2015 11:52 PM, Willem de Bruijn wrote: >>> >>> if (sock->type == SOCK_DGRAM) { >>> - err = dev_hard_header(skb, dev, ntohs(proto), addr, >>> - NULL, tp_len); >>> + /* In DGRAM sockets, we expect struct sockaddr_ll was >>> filled >>> + * via struct msghdr, so we have dest mac and >>> skb->protocol. >>> + * Otherwise there's not too much useful things we can do >>> in >>> + * this flush run. >>> + */ >>> + err = dev_hard_header(skb, dev, ntohs(skb->protocol), >>> addr, >>> + NULL, tp_len); >> >> >> This change is not really necessary. > > > Sure agreed, I found it helpful though. Don't mind removing it. > >>> if (unlikely(err < 0)) >>> return -EINVAL; >>> - } else if (dev->hard_header_len) { >> >> >> Why remove the check on hard_header_len? > > > Hmm, the patch doesn't remove the check (it's moved further below). > >>> - if (ll_header_truncated(dev, tp_len)) >>> - return -EINVAL; >>> + } else { >>> + /* If skb->protocol is still 0, try to infer/guess it. >>> Might >>> + * not be fully reliable in the sense that a user could >>> still >>> + * change/race data afterwards, but on the other hand the >>> proto >> >> >> The race goes away when probing it after the copy in skb_store_bits. >> Then it is also certain that tp_len is long enough to hold the entire >> link layer header. > > > The skb_store_bits() is only done in case we do have a dev->hard_header_len > or in case where we run into a possible situation where we have the > additional > 4 bytes on a full frame. In that case we need to check them properly, which > requires copying, otherwise we don't copy any header.
I assumed that hard_header_len has to be non-zero if there is a link layer header to probe. If we only intend to implement probing in the case of Ethernet, then it certainly holds. > >>> + * can be set arbitrarily anyways. We only need to take >>> care >>> + * in case of extra large VLAN frames. >>> + */ >>> + if (!skb->protocol && tp_len >= ETH_HLEN) >>> + skb->protocol = ((struct ethhdr *)data)->h_proto; >> >> >> Packet sockets are not restricted to link layer of type Ethernet. >> >> There are a few other points in this file that also cast mac header >> to eth_hdr(skb). > > > Ok, the set doesn't address this assumption which we have elsewhere, too. > Do you suggest to also check on dev->type for these cases? Yes. If I'm right, then the other cases have to be fixed, too. One of the eth_hdr(skb) calls was introduced by patch 09effa67a18d, where the deleted code shows how it is safely restricted to ethernet: - if (dev->type == ARPHRD_ETHER) { - skb->protocol = eth_type_trans(skb, dev); - if (skb->protocol == htons(ETH_P_8021Q)) > > Thanks, > Daniel > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html