From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754199AbdJIM1v (ORCPT ); Mon, 9 Oct 2017 08:27:51 -0400 Received: from s3.sipsolutions.net ([144.76.63.242]:39168 "EHLO sipsolutions.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751241AbdJIM1t (ORCPT ); Mon, 9 Oct 2017 08:27:49 -0400 Message-ID: <1507552064.26041.50.camel@sipsolutions.net> Subject: Re: [PATCH] netlink: do not set cb_running if dump's start() errs From: Johannes Berg To: "Jason A. Donenfeld" , davem@davemloft.net, Netdev , linux-kernel@vger.kernel.org Date: Mon, 09 Oct 2017 14:27:44 +0200 In-Reply-To: <1507550326.26041.39.camel@sipsolutions.net> References: <20171009115648.25989-1-Jason@zx2c4.com> <1507550326.26041.39.camel@sipsolutions.net> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.26.0-1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Just decided to take another look: On Mon, 2017-10-09 at 13:58 +0200, Johannes Berg wrote: > On Mon, 2017-10-09 at 13:56 +0200, Jason A. Donenfeld wrote: > > > @@ -2266,16 +2266,17 @@ int __netlink_dump_start(struct sock *ssk, > > struct sk_buff *skb, > > cb->min_dump_alloc = control->min_dump_alloc; > > cb->skb = skb; > > > > + if (cb->start) { > > + ret = cb->start(cb); > > + if (ret) > > + goto error_unlock; > > + } > > + > > nlk->cb_running = true; > > > > mutex_unlock(nlk->cb_mutex); > > Hmm. Now start is invoked with the mutex held, I'm not sure it > actually _matters_, but that should probably be reviewed and > mentioned in the commit log? It sort of seems designed to run ->start outside the lock, otherwise we wouldn't really have to acquire it again in netlink_dump() but could just keep it across the call (with some locking changes in netlink_recvmsg())? Then again, clearly none of the (few) existing users actually care. Btw - we should (separately) also remove "start" from struct netlink_callback, it's only ever used within this function and we can use control->start instead of cb->start here. johannes