netdev - Re: error handling for dev_mc_sync (__dev_addr

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]

Message-Id: <1244558606.4672.25.camel@johannes.local>
Date:	Tue, 09 Jun 2009 16:43:26 +0200
From:	Johannes Berg <johannes@...solutions.net>
To:	Patrick McHardy <kaber@...sh.net>
Cc:	netdev <netdev@...r.kernel.org>
Subject: Re: error handling for dev_mc_sync (__dev_addr_add)

On Tue, 2009-06-09 at 16:36 +0200, Patrick McHardy wrote:
> Johannes Berg wrote:
> > I can't seem to figure out how to handle errors from dev_mc_sync(). I
> > can see why it fails, looking at __dev_addr_add(), because memory
> > allocations can fail. However, it seems that __dev_addr_sync() will then
> > leave the netdev in a state where some addresses were added, but others
> > were not. And ndo_set_multicast_list() cannot even return an error code.
> > 
> > So how am I supposed to handle the error? Isn't it possible that
> > dev_mc_unsync() will then cause trouble because it removes something
> > that wasn't actually added? OTOH, there always is da->synced, but then
> > __dev_addr_sync() confuses me -- why does it not increment da->da_users?
> 
> It does:
> 
> 	if (!da->da_synced) {
> 		err = __dev_addr_add(to, to_count,
> 				     da->da_addr, da->da_addrlen, 0);
> 		if (err < 0)
> 			break;
> 		da->da_synced = 1;
> 		da->da_users++;
> 
> The problem on errors is that it can't determine which addresses
> were added in this run, and which were previously. So it can't undo
> just the actions of the last run. A generation count for da_synced
> could be used to fix that, but it would need to be bigger than the
> currently used u8.

Hmm, ok, but in the da_synced case why is da_users not incremented? I
don't claim to understand any of this code though :)

> But that wouldn't fix the fundamental problem that almost no callers
> of dev_mc_add checks for errors, so even if you abort and clean up
> properly, the higher layers will take it as success and not retry.

True. Well, dev_mc_add() isn't exactly called a lot.

> > Basically what I need is the patch below to maintain a multicast list,
> > but I'm wondering if it's all correct that way and what error handling I
> > need and am currently lacking if I ignore the return value of
> > dev_mc_sync().
> 
> Adding proper error handling looks like a bigger task. First we
> need all the callers of multicast address manipulation functions
> to actually check the return value and perform some kind of
> recovery on failure - which might not be possible in all cases,
> I'm not sure (IPv6). 

Right.

> Then we need to be able to propagate errors
> from ->set_multicast_list() and abort address list changes when
> synchronization fails - which is not particulary complicated, but
> requires touching a lot of drivers.

I think you're overstating? Regular drivers should need to be changed,
would they, except for adding changing 'return' to 'return 0;' and
adding a 'return 0;' at the end which is a quite simple spatch I'd
think.

Not that I want to do that now, I'm just confused by the semantics here,
and the lack of error handling. Additionally, I'm worried if that might
be causing the occasional 'multicast leaked' messages I was seeing, but
I doubt it.

johannes

Download attachment "signature.asc" of type "application/pgp-signature" (802 bytes)