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: <4A2E7376.4080407@trash.net>
Date:	Tue, 09 Jun 2009 16:36:38 +0200
From:	Patrick McHardy <kaber@...sh.net>
To:	Johannes Berg <johannes@...solutions.net>
CC:	netdev <netdev@...r.kernel.org>
Subject: Re: error handling for dev_mc_sync (__dev_addr_add)

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.

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.

> 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). 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.
--
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