error handling for dev_mc_sync (__dev_addr_add)

error handling for dev_mc_sync (__dev_addr_add)

[Johannes Berg]

Hi,

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?

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

johannes

--- wireless-testing.orig/include/linux/netdevice.h	2009-06-09 11:49:10.000000000 +0200
+++ wireless-testing/include/linux/netdevice.h	2009-06-09 11:54:41.000000000 +0200
@@ -1810,6 +1810,9 @@ extern int		dev_set_allmulti(struct net_
 extern void		netdev_state_change(struct net_device *dev);
 extern void		netdev_bonding_change(struct net_device *dev);
 extern void		netdev_features_change(struct net_device *dev);
+/* locking of to/to_count must be caller-provided */
+extern int		mc_sync_from_dev(struct dev_addr_list **to, int *to_count, struct net_device *from);
+extern void		mc_unsync_from_dev(struct dev_addr_list **to, int *to_count, struct net_device *from);
 /* Load a device via the kmod */
 extern void		dev_load(struct net *net, const char *name);
 extern void		dev_mcast_init(void);
--- wireless-testing.orig/net/core/dev_mcast.c	2009-06-09 11:50:37.000000000 +0200
+++ wireless-testing/net/core/dev_mcast.c	2009-06-09 12:26:17.000000000 +0200
@@ -155,6 +155,52 @@ void dev_mc_unsync(struct net_device *to
 }
 EXPORT_SYMBOL(dev_mc_unsync);
 
+
+/**
+ *	mc_sync_from_dev - Synchronize device's multicast list to another list
+ *	@to: destination list
+ *	@to_count: destination list count
+ *	@from: source device
+ *
+ * 	Add newly added addresses to the destination device and release
+ * 	addresses that have no users left. The source device must be
+ * 	locked by netif_tx_lock_bh, and the destination list must be locked
+ *	in whatever way the caller manages it.
+ *
+ *	This function is intended to be called from the dev->set_multicast_list
+ *	or dev->set_rx_mode function of layered software devices.
+ */
+int mc_sync_from_dev(struct dev_addr_list **to, int *to_count,
+		     struct net_device *from)
+{
+	return __dev_addr_sync(to, to_count,
+			       &from->mc_list, &from->mc_count);
+}
+EXPORT_SYMBOL(mc_sync_from_dev);
+
+
+/**
+ * 	mc_unsync_from_dev - Remove synchronized addresses from the destination list
+ *	@to: destination list
+ *	@to_count: destination list count
+ *	@from: source device
+ *
+ * 	Remove all addresses that were added to the destination list by
+ * 	mc_sync_from_dev(). This function is intended to be called from
+ *	the * 	dev->stop function of layered software devices. The
+ *	destination list must be locked in whatever way the caller
+ *	manages it.
+ */
+void mc_unsync_from_dev(struct dev_addr_list **to, int *to_count,
+			struct net_device *from)
+{
+	netif_addr_lock_bh(from);
+	__dev_addr_unsync(to, to_count,
+			  &from->mc_list, &from->mc_count);
+	netif_addr_unlock_bh(from);
+}
+EXPORT_SYMBOL(mc_unsync_from_dev);
+
 #ifdef CONFIG_PROC_FS
 static int dev_mc_seq_show(struct seq_file *seq, void *v)
 {

Re: error handling for dev_mc_sync (__dev_addr_add)

[Patrick McHardy]

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.

Re: error handling for dev_mc_sync (__dev_addr_add)

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2911 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: error handling for dev_mc_sync (__dev_addr_add)

[Patrick McHardy]

Johannes Berg wrote:
> On Tue, 2009-06-09 at 16:36 +0200, Patrick McHardy wrote:
>>> 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 :)

Its kind of a mess. In my excuse, it was even worse before the
synchronization functions were added :)

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.

>> 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?

Am I?

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

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

Re: error handling for dev_mc_sync (__dev_addr_add)

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2190 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: error handling for dev_mc_sync (__dev_addr_add)

[Patrick McHardy]

Johannes Berg wrote:
> On Tue, 2009-06-09 at 17:00 +0200, Patrick McHardy wrote:
> 
>>> 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)

- void
+ int

> {
> ...
> -return;
> +return 0;
> ...
> }
> ----- %< -----
> 
> Haven't really tested it though but it seems to do the correct thing for
> all drivers I looked at.

In case this also adds a return statement at the end if none is
there already, it looks fine.

Re: error handling for dev_mc_sync (__dev_addr_add)

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 820 bytes --]

On Tue, 2009-06-09 at 19:10 +0200, Patrick McHardy wrote:

> > ----- >% -----
> > @ 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)
> 
> - void
> + int

Indeed.

> > {
> > ...
> > -return;
> > +return 0;
> > ...
> > }
> > ----- %< -----
> > 
> > Haven't really tested it though but it seems to do the correct thing for
> > all drivers I looked at.
> 
> In case this also adds a return statement at the end if none is
> there already, it looks fine.

Yes, it does. "All" that remains to be done then is to actually fix the
few users of dev_mc_sync(), and the callers, but that's the real work :)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]