Re: Relayfs Question: Use of relay_reset(). Potential race?

Kingsley Cheung
2005-04-19T01:16:15Z

On Mon, Apr 18, 2005 at 10:56:57AM -0500, Tom Zanussi wrote:
> Kingsley Cheung writes:
> > On Wed, Mar 23, 2005 at 08:02:54PM +1100, kingsley@aurema.com wrote:
> > > Hi
> > >
> > > I'm using relayfs to relay data from a kernel module to user space on
> > > a SuSE 2.6.5 kernel. I'm not absolutely sure what version of relayfs
> > > has been back ported to it.
> >
> > Hi Tom,
> >
> > Could you please have a look at the following use of relay_reset() in
> > a kernel module as follows (compiled against pre-redux relayfs):
(snip)
> > Is that legitimate? The reason I ask is because I've been seeing
>
> Yes, you should be able to reset the channel here, since at that point
> it's been closed.
>
> > garbled oopses with keventd and I've narrowed it to two things:
> >
> > 1) Inadequate locking on my part in the kernel module, which I have
> > addressed separately.
> >
> > 2) A race with relay_reset() and keventd, which is probably of
> > interest to you if you're still maintaining the pre-redux patches.
> >
> > The race is due to the use of INIT_WORK in _reset_relay():
> >
> > INIT_WORK(&rchan->wake_readers, NULL, NULL);
> > INIT_WORK(&rchan->wake_writers, NULL, NULL);
> >
> > However, at the time relay_reset() is called, it is possible that
> > these work structures are still being used by keventd when under heavy
> > loads. The workaround I've used to fix this is to call
> > flush_scheduled_work() before calling reset_relay() in the kernel
> > module. Perhaps that needs to be called in relay_reset() or
> > _relay_reset()?

Tom,

Thanks for the prompt response.

> Yes, flush_scheduled_work() should probably be called from
> __relay_reset() - thanks for catching this and suggesting the fix.

My pleasure.

> BTW, I've updated the old relayfs patch with your previous fixes and
> ported it to 2.6.11 - it hasn't appeared yet on the opersys website,
> so let me know if you want it and I'll send it, or I can just send you
> a new version after I make these changes...

Ok, good to hear. I've been using the old patch against an earlier
version of the kernel, so I've no rush for a patch against 2.6.11 -
you can take your time with these new changes :)

> > As well I'm not sure about the uses of INIT_WORK in
> > _relay_realloc_buffer() and relay_release() - perhaps they need
> > attention too. I understand, however, that flush_schedule_work()
>
> I'll take a look at this too - looks like there might be a potential
> problem if a channel was closed while a resize was in progress. I
> don't think anyone's ever actually used resizing, but it should be
> fixed nonetheless.
>
> Thanks,
>
> Tom
>

Right. Thanks again for looking at it.

--
Kingsley