netdev - Re: [PATCH net 1/8] amt: use workqueue for gateway side message handling

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]

Message-ID: <6d75ddde-c7aa-d134-4d92-3aaa1f96f717@gmail.com>
Date:   Fri, 15 Jul 2022 21:34:51 +0900
From:   Taehee Yoo <ap420073@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, pabeni@...hat.com, edumazet@...gle.com,
        netdev@...r.kernel.org
Subject: Re: [PATCH net 1/8] amt: use workqueue for gateway side message
 handling

Hi Jakub,
Thank you so much for your review!

On 7/14/22 12:55, Jakub Kicinski wrote:
 > On Tue, 12 Jul 2022 10:57:07 +0000 Taehee Yoo wrote:
 >> @@ -2392,12 +2429,14 @@ static bool 
amt_membership_query_handler(struct amt_dev *amt,
 >>   	skb->pkt_type = PACKET_MULTICAST;
 >>   	skb->ip_summed = CHECKSUM_NONE;
 >>   	len = skb->len;
 >> +	rcu_read_lock_bh();
 >>   	if (__netif_rx(skb) == NET_RX_SUCCESS) {
 >>   		amt_update_gw_status(amt, AMT_STATUS_RECEIVED_QUERY, true);
 >>   		dev_sw_netstats_rx_add(amt->dev, len);
 >>   	} else {
 >>   		amt->dev->stats.rx_dropped++;
 >>   	}
 >> +	rcu_read_unlock_bh();
 >>
 >>   	return false;
 >>   }
 >
 > The RCU lock addition looks potentially unrelated?
 >

I checked that RCU is not necessary here.
I tested local_bh_disable(), and it works well. no splats appeared.
So, as Paolo suggested, I will use local_bh_disable() instead of 
rcu_read_lock_bh() in the v2.

 >> @@ -2892,10 +3007,21 @@ static int amt_dev_stop(struct net_device *dev)
 >>   	struct amt_dev *amt = netdev_priv(dev);
 >>   	struct amt_tunnel_list *tunnel, *tmp;
 >>   	struct socket *sock;
 >> +	struct sk_buff *skb;
 >> +	int i;
 >>
 >>   	cancel_delayed_work_sync(&amt->req_wq);
 >>   	cancel_delayed_work_sync(&amt->discovery_wq);
 >>   	cancel_delayed_work_sync(&amt->secret_wq);
 >> +	cancel_work_sync(&amt->event_wq);
 >
 > Are you sure the work will not get scheduled again?
 > What has stopped packet Rx at this point?

I expected that RX was already stopped then ->ndo_stop() can be called.
But as you pointed out, it isn't.
In order to ensure that no more RX concurrently, amt_rcv() should not be 
called.
So, I think cancel_delayed_work() should be called after setting null to 
the amt socket in amt_dev_stop().
code looks like:
    RCU_INIT_POINTER(amt->sock, NULL);
    synchronize_net();
    cancel_delayed_work(&amt->event_wq).

 >
 >> +	for (i = 0; i < AMT_MAX_EVENTS; i++) {
 >> +		skb = amt->events[i].skb;
 >> +		if (skb)
 >> +			kfree_skb(skb);
 >> +		amt->events[i].event = AMT_EVENT_NONE;
 >> +		amt->events[i].skb = NULL;
 >> +	}
 >>
 >>   	/* shutdown */
 >>   	sock = rtnl_dereference(amt->sock);
 >> @@ -3051,6 +3177,8 @@ static int amt_newlink(struct net *net, struct 
net_device *dev,
 >>   		amt->max_tunnels = AMT_MAX_TUNNELS;
 >>
 >>   	spin_lock_init(&amt->lock);
 >> +	amt->event_idx = 0;
 >> +	amt->nr_events = 0;
 >
 > no need to init member of netdev_priv() to 0, it's zalloc'ed
 >

Thanks a lot for pointing it out.
This is my mistake.
If an amt interface is down and then up again, it would have incorrect 
values of event_idx and nr_events.
Because there is no init for these values.
So, init for these variable in ->ndo_open() or ->ndo_stop() is needed.
Initilization in the ->ndo_newlink() can't fix anything.

As a test, there is a bug certainly.
So, I will move it to ->ndo_open().

 >>   	amt->max_groups = AMT_MAX_GROUP;
 >>   	amt->max_sources = AMT_MAX_SOURCE;
 >>   	amt->hash_buckets = AMT_HSIZE;

I will send the v2 patch after some tests!

Thanks a lot for your review!
Taehee Yoo