> Gur Stavi wrote:
> > PACKET socket can retain its fanout membership through link down and up
> > and leave a fanout while closed regardless of link state.
> > However, socket was forbidden from joining a fanout while it was not
> > RUNNING.
> >
> > This patch allows PACKET socket to join fanout while not RUNNING.
> >
> > The previous test for RUNNING also implicitly tested that the socket is
> > bound to a device. An explicit test of ifindex was added instead.
> >
> > Signed-off-by: Gur Stavi <gur.stavi@xxxxxxxxxx>
> > ---
> > net/packet/af_packet.c | 35 +++++++++++++++++++++--------------
> > 1 file changed, 21 insertions(+), 14 deletions(-)
> >
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index f8942062f776..8137c33ab0fd 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -1843,26 +1843,29 @@ static int fanout_add(struct sock *sk, struct
> fanout_args *args)
> > match->prot_hook.ignore_outgoing = type_flags &
> PACKET_FANOUT_FLAG_IGNORE_OUTGOING;
> > list_add(&match->list, &fanout_list);
> > }
> > - err = -EINVAL;
> >
> > spin_lock(&po->bind_lock);
> > - if (packet_sock_flag(po, PACKET_SOCK_RUNNING) &&
> > - match->type == type &&
> > - match->prot_hook.type == po->prot_hook.type &&
> > - match->prot_hook.dev == po->prot_hook.dev) {
> > + if (po->ifindex == -1 || po->num == 0) {
>
> This patch is more complex than it needs to be.
>
> No need to block the case of ETH_P_NONE or not bound to a socket.
ETH_P_NONE was blocked before as well.
packet_do_bind will not switch socket to RUNNING when proto is 0.
if (proto == 0 || !need_rehook)
goto out_unlock;
Same for packet_create.
So the old condition could only pass the RUNNING condition if proto
was non-zero.
The new condition is exactly equivalent except for allowing IFF_UP
to be cleared in the bound device.
Yes, the same result could be achieved with a FEW less line changes
but I think that the new logic is more readable where every clause
explains itself with a comment instead of constructing one large if
statement. And since the solution does add another nested if for the
RUNNING the extra indentation started to look ugly.
>
> I would have discussed that in v2, but you already respun.
>
> > + /* Socket can not receive packets */
> > + err = -ENXIO;
> > + } else if (match->type != type ||
> > + match->prot_hook.type != po->prot_hook.type ||
> > + match->prot_hook.dev != po->prot_hook.dev) {
> > + /* Joining an existing group, properties must be identical */
> > + err = -EINVAL;
> > + } else if (refcount_read(&match->sk_ref) >= match->max_num_members)
> {
> > err = -ENOSPC;
> > - if (refcount_read(&match->sk_ref) < match->max_num_members) {
> > + } else {
> > + /* Paired with packet_setsockopt(PACKET_FANOUT_DATA) */
> > + WRITE_ONCE(po->fanout, match);
> > + po->rollover = rollover;
> > + rollover = NULL;
> > + refcount_set(&match->sk_ref, refcount_read(&match->sk_ref) +
> 1);
> > + if (packet_sock_flag(po, PACKET_SOCK_RUNNING)) {
> > __dev_remove_pack(&po->prot_hook);
> > -
> > - /* Paired with packet_setsockopt(PACKET_FANOUT_DATA) */
> > - WRITE_ONCE(po->fanout, match);
> > -
> > - po->rollover = rollover;
> > - rollover = NULL;
> > - refcount_set(&match->sk_ref, refcount_read(&match-
> >sk_ref) + 1);
> > __fanout_link(sk, po);
> > - err = 0;
> > }
> > + err = 0;
> > }
> > spin_unlock(&po->bind_lock);
> >
> > @@ -3452,8 +3455,12 @@ static int packet_create(struct net *net, struct
> socket *sock, int protocol,
> > po->prot_hook.af_packet_net = sock_net(sk);
> >
> > if (proto) {
> > + /* Implicitly bind socket to "any interface" */
> > + po->ifindex = 0;
> > po->prot_hook.type = proto;
> > __register_prot_hook(sk);
> > + } else {
> > + po->ifindex = -1;
> > }
> >
> > mutex_lock(&net->packet.sklist_lock);
> > --
> > 2.45.2
> >
>
