On Tue, 2007-09-04 at 12:45 -0700, Thirumalai Srinivasan wrote:
> Garrett,
>
> Can there be 2 concurrent threads executing in the driver, on the same
> mac endpoint, one of which is doing a call to mac_unregister(), while
> another thread is doing a call to i_mac_notify() (at the same time) ? If
> so we have a race. If mac_unregister() finishes successfully before
> i_mac_notify even starts off, then i_mac_notify() may access a freed
> mip while trying to check for mi_disabled.
Hmm.... this is a good point.
>
> Iam not talking about the asynchronous i_mac_notify_thread(), and
> synchronizing that with mac_unregister() is the job of the mac framework.
>
> Iam saying drivers have to ensure that they are pretty much
> single-threaded (with respect to mac calls) while they call
> mac_unregister(). Iam hoping that is already the case today.
Well, I know at least a couple of drivers that don't obey this rule.
(Every driver I've looked at breaks it... e1000g, bge, eri, hme, rtls,
mxfe, etc.)
This is one problem with having mac_unregister free the mac_impl_t.
I think the GLDv2 was better here... it had explicit routines to
allocate and free the soft state, independent of the routines to
register and unregister. For some reason someone decided to get
"clever" with the GLDv3...
Fixing this particular bug is "non-trivial" without introducing
something like a separate API for freeing the mac_impl_t. The reason
for this is that each driver will have its own set of locks, whatever,
to go single threaded.
Normally "shutting down" threads, timeouts, etc. isn't done until
*after* the instance has been "unregistered" from use. (That way the
driver knows that no consumers for the interface are still present, so
shutting down these async methods, etc. is safe.)
We could use some sort of heuristic, where the mac_info_t's are not
freed but retained for a while. Perhaps a DACF-like hook on device
removal? What a hack.
The best solution is a change to the Nemo API. Another reason I'm glad
that Nemo isn't yet public. I sent more about that a short while ago.
Note that this problem predates the review I've got out, and the problem
exists in both the code before and after modification.
-- Garrett
>
> Thirumalai
>
> Garrett D'Amore wrote:
>
> >
> >
> >>>>>
> >>>>>
> >>More than the 'mi_disabled', the real requirement here is for drivers to
> >>make sure that they call mac_unregister() only after all upcall threads
> >>and notifications are done.
> >>
> >>
> >
> >They can't now when the thread is complete, because it runs
> >asynchronously. However, they should not be asynchronously submitting
> >new calls to i_mac_notify at this time. (Old calls may still be
> >pending/in-progress however.)
> >
> >
> >
> >> If there is a concurrent call to
> >>i_mac_notify(), while mac_unregister() is called (both by the driver),
> >>it is possible that by the time i_mac_notify even starts and tries to
> >>examine mip->mi_disabled, mac_unregister() is already finished and has
> >>freed up the mac_impl_t.
> >>
> >>
> >
> >Yes, that is true. But, the i_mac_notify_thread may still be running.
> >And it enforces its protection by holding the rwlock for the
> >i_mac_impl_lock.
> >
> >
> >
>
_______________________________________________
networking-discuss mailing list
[email protected]
