netdev - Re: [RFC] RF Kill

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]

Date:	Thu, 26 Apr 2007 08:52:55 -0400
From:	"Dmitry Torokhov" <dmitry.torokhov@...il.com>
To:	"David Miller" <davem@...emloft.net>
Cc:	netdev@...r.kernel.org, ivdoorn@...il.com, linville@...driver.com,
	akpm@...ux-foundation.org
Subject: Re: [RFC] RF Kill

On 4/26/07, David Miller <davem@...emloft.net> wrote:
> From: Dmitry Torokhov <dtor@...ightbb.com>
> Date: Tue, 10 Apr 2007 02:29:31 -0400
>
> > On Tuesday 10 April 2007 01:58, Dmitry Torokhov wrote:
> > > Hi,
> > >
> > > This is a modified version of rfkill patch that provides infrastructure
> > > for controlling state of RF transmitters found on various cards.
> >
> > Well, Andrew found bunch of issues with the patch so here is an
> > updated version...
>
> Patch applied, although one part of the locking is slightly
> suspect:
>
> > +static void rfkill_task_handler(struct work_struct *work)
> > +{
> > +     struct rfkill_task *task = container_of(work, struct rfkill_task, work);
> > +     enum rfkill_state state;
> > +
> > +     mutex_lock(&task->mutex);
> > +
> > +     spin_lock_irq(&task->lock);
> > +     state = task->desired_state;
> > +     spin_unlock_irq(&task->lock);
> > +
> > +     if (state != task->current_state) {
> > +             rfkill_switch_all(task->type, state);
> > +             task->current_state = state;
> > +     }
> > +
> > +     mutex_unlock(&task->mutex);
> > +}
>
> I applied this, but...
>
> That lock around the read doesn't make any sense, reads
> are atomic on all SMP processors. You're not going to
> see a partial word-update if you take away that lock so
> it isn't doing anything.
>

Ah, OK. I was always concerned with partial word updates but if we
have this guarantee this makes things simplier. I will remove the lock
there and I will add a comment that a temp is still needed since
desired_state may be changed from other thread.

> If locking is really needed here, it probably need to protect
> the whole read-modify-write operation transferring the
> desired_state to the current_state.
>
> In another code block, this ->desired_state thing is
> treated like a boolean instead of the enumeration that
> it is supposed to be:

It is boolean enumeration with values of 0 and 1 ;) and so state =
!state should work.

-- 
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html