Date: Fri, 17 Jul 2020 12:03:11 +0200 From: Marko Zec <zec@fer.hr> To: John-Mark Gurney <jmg@funkthat.com> Cc: freebsd-net@FreeBSD.org Subject: Re: IF_DRV_PREPEND unlocked? Message-ID: <20200717120311.59377e0d@x23> In-Reply-To: <20200716185629.GT4213@funkthat.com> References: <20200715232624.GR4213@funkthat.com> <20200716072622.5fa35ba2@x23> <20200716074917.04445daa@x23> <20200716185629.GT4213@funkthat.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 16 Jul 2020 11:56:29 -0700 John-Mark Gurney <jmg@funkthat.com> wrote: > Marko Zec wrote this message on Thu, Jul 16, 2020 at 07:49 +0200: > > On Thu, 16 Jul 2020 07:26:22 +0200 > > Marko Zec <zec@fer.hr> wrote: > > > > > On Wed, 15 Jul 2020 16:26:25 -0700 > > > John-Mark Gurney <jmg@funkthat.com> wrote: > > > > > > > I happen to be looking at the implementation of > > > > IFQ_DRV_PREPEND, and unlike IFQ_DRV_DEQUEUE, it doesn't obtain > > > > the lock when manipulating the ifq. I took a brief look at a > > > > few drivers, and it looks like some of them expect that > > > > _PREPEND lock the Q like _DEQUEUE does. > > > > > > > > This is likely not an issue often, since it's an error path that > > > > likely rarely happens, but we should fix it. > > > > > > > > Should we just add the IFQ_LOCK/_UNLOCK to the macro? > > > > > > > > Comments or thoughts? > > > > > > I also have a hard time understanding the semantics of IFQ_DRV_* > > > macros: per altq(9) they appear to be variants of their IFQ_ > > > counterparts which are intended to be protected by some other > > > mechanism rather than IFQ_LOCK, but in some instances this isn't > > > the case, as they do grab the IFQ_LOCK. > > > > Hmm it's not that unclear after looking better: ifq_drv_* parts of > > struct ifaltq should be protected by some external mechanisms when > > using IFQ_DRV_ macros, and ifq_ parts are still protected by > > ifq_mtx. So it doesn't look like IFQ_LOCK/_UNLOCK should be added to > > IFQ_DRV_PREPEND()... > > Ahh, I was miss reading the macros on what was protected. Yes, it > does look like the ifq_drv_* fields are supposed to be protected by > another lock... We were both too fast to conclude that all IFQ_DRV_* are OK, but now I'm not sure any more: #define IFQ_DRV_IS_EMPTY(ifq) \ (((ifq)->ifq_drv_len == 0) && ((ifq)->ifq_len == 0)) So, if per altq(9) the contract is that with IFQ_DRV_* the ifq_drv_* fields should be protected by some caller-provided mechanism, while the other ifq_* fields will be implictly protected by ifq_mtx, how can accessing ifw_len without holding ifq_mtx in the above example be safe? Marko > > I just now found that altq(9) documents this interface... I had > assumed it as undocumented after man IFQ_DRV_DEQUEUE didn't return > results.. > > Looks like I REALLY do need to complete my script to make sure that > ALL Nm's and related pages have MLINKs. ALTQ.9 is missing 16 links > to the functions it documents... > > Just as a reminder to people who write docs, please, make sure you > link ALL of the function in your docs... Otherwise your docs won't > be as easy to find.. >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20200717120311.59377e0d>