2010/12/14 Michał Mirosław <[email protected]>: > 2010/12/14 Mike Waychison <[email protected]>: >> The netconsole driver currently doesn't do any locking over its >> configuration fields. This can cause problems if we were to ever have >> concurrent writing to fields while somebody is enabling the service. >> >> For simplicity, this patch extends targets_list_lock to cover all >> configuration fields within the targets. Macros are also added here to >> wrap accessors so that we check whether the target has been enabled with >> locking handled. >> >> Signed-off-by: Mike Waychison <[email protected]> >> Acked-by: Matt Mackall <[email protected]> >> --- >> drivers/net/netconsole.c | 114 >> ++++++++++++++++++++++++++-------------------- >> 1 files changed, 64 insertions(+), 50 deletions(-) >> >> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c >> index c87a49e..6e16888 100644 >> --- a/drivers/net/netconsole.c >> +++ b/drivers/net/netconsole.c >> @@ -327,6 +327,7 @@ static ssize_t store_enabled(struct netconsole_target >> *nt, >> const char *buf, >> size_t count) >> { >> + unsigned long flags; >> int err; >> long enabled; >> >> @@ -335,6 +336,10 @@ static ssize_t store_enabled(struct netconsole_target >> *nt, >> return enabled; >> >> if (enabled) { /* 1 */ >> + spin_lock_irqsave(&target_list_lock, flags); >> + if (nt->enabled) >> + goto busy; >> + spin_unlock_irqrestore(&target_list_lock, flags); >> > > This looks wrong. Unless there is another lock or mutex covering this > function, at this point (after spin_unlock_irqrestore()) another > thread might set nt->enabled = 1. >
Agreed that this looks wrong :) It is fixed in the next patch where a state machine is introduced to replace the binary flag nt->enabled. The code before this patch had the a very similar problem in that a target could be enabled twice. store_enabled() would call netpoll_setup() the second time without checking to see if it was already enabled. -- To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
