From mboxrd@z Thu Jan 1 00:00:00 1970 From: Oliver Hartkopp Subject: Re: [PATCH] can: Fix kernel panic at security_sock_rcv_skb Date: Thu, 12 Jan 2017 09:22:17 +0100 Message-ID: <0e458012-e97b-298d-296e-ed25e35689a0@hartkopp.net> References: <1484202799-7287-1-git-send-email-shuo.a.liu@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Cc: yanmin_zhang@linux.intel.com, shuox.liu@gmail.com, Zhang Yanmin , "He, Bo" , Marc Kleine-Budde , "David S. Miller" , "open list:CAN NETWORK LAYER" , "open list:NETWORKING [GENERAL]" To: Liu ShuoX , linux-kernel@vger.kernel.org Return-path: In-Reply-To: <1484202799-7287-1-git-send-email-shuo.a.liu@intel.com> Sender: linux-can-owner@vger.kernel.org List-Id: netdev.vger.kernel.org On 01/12/2017 07:33 AM, Liu ShuoX wrote: > From: Zhang Yanmin > > The patch is for fix the below kernel panic: > BUG: unable to handle kernel NULL pointer dereference at (null) > IP: [] selinux_socket_sock_rcv_skb+0x65/0x2a0 > > Call Trace: > > [] security_sock_rcv_skb+0x4c/0x60 > [] sk_filter+0x41/0x210 > [] sock_queue_rcv_skb+0x53/0x3a0 > [] raw_rcv+0x2a3/0x3c0 > [] can_rcv_filter+0x12b/0x370 > [] can_receive+0xd9/0x120 > [] can_rcv+0xab/0x100 > [] __netif_receive_skb_core+0xd8c/0x11f0 > [] __netif_receive_skb+0x24/0xb0 > [] process_backlog+0x127/0x280 > [] net_rx_action+0x33b/0x4f0 > [] __do_softirq+0x184/0x440 > [] do_softirq_own_stack+0x1c/0x30 > > [] do_softirq.part.18+0x3b/0x40 > [] do_softirq+0x1d/0x20 > [] netif_rx_ni+0xe5/0x110 > [] slcan_receive_buf+0x507/0x520 > [] flush_to_ldisc+0x21c/0x230 > [] process_one_work+0x24f/0x670 > [] worker_thread+0x9d/0x6f0 > [] ? rescuer_thread+0x480/0x480 > [] kthread+0x12c/0x150 > [] ret_from_fork+0x3f/0x70 > > The sk dereferenced in panic has been released. After the rcu_call in > can_rx_unregister, receiver was protected by RCU but inner data was > not, then later sk will be freed while other CPU is still using it. > We need wait here to make sure sk referenced via receiver was safe. > > => security_sk_free > => sk_destruct > => __sk_free > => sk_free > => raw_release > => sock_release > => sock_close > => __fput > => ____fput > => task_work_run > => exit_to_usermode_loop > => syscall_return_slowpath > => int_ret_from_sys_call > > Signed-off-by: Zhang Yanmin > Signed-off-by: He, Bo > Signed-off-by: Liu Shuo A > --- > net/can/af_can.c | 14 ++++++++------ > net/can/af_can.h | 1 - > 2 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/net/can/af_can.c b/net/can/af_can.c > index 1108079..fcbe971 100644 > --- a/net/can/af_can.c > +++ b/net/can/af_can.c > @@ -517,10 +517,8 @@ int can_rx_register(struct net_device *dev, canid_t can_id, canid_t mask, > /* > * can_rx_delete_receiver - rcu callback for single receiver entry removal > */ > -static void can_rx_delete_receiver(struct rcu_head *rp) > +static void can_rx_delete_receiver(struct receiver *r) > { > - struct receiver *r = container_of(rp, struct receiver, rcu); > - > kmem_cache_free(rcv_cache, r); > } > > @@ -595,9 +593,13 @@ void can_rx_unregister(struct net_device *dev, canid_t can_id, canid_t mask, > out: > spin_unlock(&can_rcvlists_lock); > > - /* schedule the receiver item for deletion */ > - if (r) > - call_rcu(&r->rcu, can_rx_delete_receiver); > + /* synchronize_rcu to wait until a grace period has elapsed, to make > + * sure all receiver's sk dereferenced by others. > + */ > + if (r) { > + synchronize_rcu(); > + can_rx_delete_receiver(r); Nitpick: When can_rx_delete_receiver() just contains kmem_cache_free(rcv_cache, r), then the function definition should be removed. But my main concern is: The reason why can_rx_delete_receiver() was introduced was the need to remove a huge number of receivers with can_rx_unregister(). When you call synchronize_rcu() after each receiver removal this would potentially lead to a big performance issue when e.g. closing CAN_RAW sockets with a high number of receivers. So the idea was to remove/unlink the receiver hlist_del_rcu(&r->list) and also kmem_cache_free(rcv_cache, r) by some rcu mechanism - so that all elements are cleaned up by rcu at a later point. Is it possible that the problems emerge due to hlist_del_rcu(&r->list) and you accidently fix it with your introduced synchronize_rcu()? Regards, Oliver > + } > } > EXPORT_SYMBOL(can_rx_unregister); > > diff --git a/net/can/af_can.h b/net/can/af_can.h > index fca0fe9..a0cbf83 100644 > --- a/net/can/af_can.h > +++ b/net/can/af_can.h > @@ -50,7 +50,6 @@ > > struct receiver { > struct hlist_node list; > - struct rcu_head rcu; > canid_t can_id; > canid_t mask; > unsigned long matches; >