Re: CVS commit: src/sys/net

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Thu, Apr 6, 2017 at 3:34 AM, Taylor R Campbell
<campbell+netbsd-source-changes-d%mumble.net@localhost> wrote:
>> Date: Wed, 5 Apr 2017 15:46:38 +0800 (+08)
>> From: Paul Goyette <paul%whooppee.com@localhost>
>>
>> @@ -3385,6 +3428,13 @@ if_mcast_op(ifnet_t *ifp, const unsigned
>>   {
>>          int rc;
>>          struct ifreq ifr;
>> +       bool need_unlock = false;
>> +
>> +       /* XXX if_ioctl_lock may or may not be held here */
>> +       if (ifp->if_ioctl_lock != NULL && !mutex_owned(ifp->if_ioctl_lock)) {
>> +               mutex_enter(ifp->if_ioctl_lock);
>> +               need_unlock = true;
>> +       }
>>
>> It is my understanding that using mutex_owned() to effect locking
>> decisions is forbidden.
>>
>> I understand the intent of doing it this way, but perhaps we should
>> revisit the callers and enforce that they always take the lock?
>
> Quite right.  Please revert this change and post a note to tech-net@
> about how to resolve the issue properly if you need help, including a
> statement of what code paths do or don't hold the ioctl lock and why
> it's hard to address them.

It tried to put the ioctl lock at upper callers and gave up doing so
because that's too complex (ioctl is easily called recursively) and
the ioctl lock for an interface doesn't fit to upper callers that
don't related to ioctl at all. So I don't think it's worthwhile to
investigate where we should lock correctly at upper callers. If we
correctly fix the issue, we should make ioctl callees MP-safe somehow.

Anyway I know the change isn't the best approach so I'll revert
the commit.

>
> From what I recall (haven't looked recently, though) the ifioctl
> locking may be broken, and may be better written with localcount(9).

That part now uses psref and if_ioctl_lock. I'm not sure localcount
suits more for that.

  ozaki-r