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: <1244567063.18481.13.camel@johannes.local>
Date:	Tue, 09 Jun 2009 19:04:22 +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 17:00 +0200, Patrick McHardy wrote:

> >> 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 :)
> 
> Its kind of a mess. In my excuse, it was even worse before the
> synchronization functions were added :)

Oh, I'm not blaming anyone :)

> The __dev_addr_sync() function is for both incremental additions and
> removals. In the da_synced (== already synced) case, it deletes the
> address if it has a only a single reference left, which means its
> only held for unsychronization, and releases the address completely
> afterwards.

Hmm, ok.

> # grep -r set_multicast_list drivers/net/ | wc -l
> 499
> 
> It probably includes some false positives, but I think its still
> quite a lot. You could of course add a new callback for those few
> drivers which actually can fail.
> 
> > 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.
> 
> Right.

Yeah, so this spatch should be sufficient:

----- >% -----
@ ndomatch @
identifier ndo, sml;
@@
struct net_device_ops ndo = {
.ndo_set_multicast_list = sml,
};
@forall@
identifier dev;
identifier ndomatch.sml;
@@
void sml(struct net_device *dev)
{
...
-return;
+return 0;
...
}
----- %< -----

Haven't really tested it though but it seems to do the correct thing for
all drivers I looked at.

> > 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.
> 
> It shouldn't cause this from what I can tell.

Ok, good, thanks.

johannes

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