Gur Stavi wrote:
Gur Stavi wrote:
If we don't care about opening up fanout groups to ETH_P_NONE, then patch v2 seems sufficient. If explicitly blocking this, the ENXIO return can be added, but ideally without touching the other lines.
I don't think that allowing ETH_P_NONE is relevant. In my opinion the 2 options that should be considered to fail fanout_add are:
- Testing proto == 0
- Testing proto == 0 || ifindex == -1
The only corner case that is caught by [2] and missed by [1] is the "unlisted" case during do_bind. It is such a rare case that probably no one will ever encounter bind "unlisted" followed by FANOUT_ADD. And this is not a dangerous corner case that leads to system crash.
However, being a purist, I see the major goal of code review to promote correctness by identifying corner cases while improving style is a secondary priority. Since we did identify this corner case in our discussion I think we should still use [2]. I don't consider the code complex. In fact, to me, the ifindex clause is a more understandable direct reason for failure than the proto which is indirect. Having the ifindex clause helps figuring out the proto clause.
It's interesting that the unlisted fix does not return ENODEV, but returns success and leaves the socket in an unbound state, equivalent to binding to ETH_P_NONE and ifindex 0. This seems surprising behavior to the caller.
On rereading that, I still do not see a purpose of special ifindex -1.
Can this code be relevant?
case NETDEV_UP: if (dev->ifindex == po->ifindex) { spin_lock(&po->bind_lock); if (po->num) register_prot_hook(sk); spin_unlock(&po->bind_lock); } break;Perhaps, although the socket failed to (re) find the device, the device is still aware of the socket and we need the ifindex condition to fail.
But the behavior is the same for ifindex -1 and 0. Devices always have an ifindex >= 1.