netdev - Re: [PATCH 04/10] AOE: use rcu to find network device

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: <e160a17e366ae2f676df822db1e62bc2@coraid.com>
Date:	Thu, 12 Nov 2009 09:33:16 -0500
From:	Ed Cashin <ecashin@...aid.com>
To:	shemminger@...tta.com, karaluh@...aluh.pl, ecashin@...aid.com,
	roel.kluin@...il.com, harvey.harrison@...il.com,
	bzolnier@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH 04/10] AOE: use rcu to find network device

Thanks again for providing this patch to help get things started.
It's very helpful.  I appreciate the way it reflects and fits into the
rest of the driver, too.

In the patch, there's a loop around getif, ejectif inside the new
aoecmd_flushnet function:

  void aoecmd_flushnet(struct aoedev *d, struct net_device *nd)
  {
  	struct aoetgt **tt, **te;
  	tt = d->targets;
  	te = tt + NTARGETS;
  	for (; tt < te && *tt; tt++) {
  		struct aoetgt *t = *tt;
  		struct aoeif *ifp;

  		while ( (ifp = getif(t, nd)) )
  			ejectif(t, ifp);
  	}
  }

... but an "if" seems appropriate, since duplicates are avoided when
network devices are added in aoecmd_cfg_rsp:

  	ifp = getif(t, skb->dev);
  	if (!ifp) {
  		ifp = addif(t, skb->dev);

If there's some other reason for it to be "while" in aoecmd_flushnet
that I'm missing, please let me know.

Regarding the new aoe_device_event function,

  /* Callback on change of state of network device. */
  static int aoe_device_event(struct notifier_block *unused,
  			    unsigned long event, void *ptr)
  {
  	struct net_device *nd = ptr;

  	if (is_aoe_netif(nd) && event == NETDEV_UNREGISTER)
  		aoedev_ejectnet(nd);

  	return NOTIFY_DONE;
  }

...  the is_aoe_netif function really answers the question, "Are we
allowed by the user to do AoE on this local network interface?" The
user can modify the list of allowable interfaces at runtime via sysfs,
as it's a module parameter.  So I don't think we can use is_aoe_netif
in the new aoe_device_event function.  We should eject a net_device in
this handler even if the user has decided not to use it for AoE.
Please let me know if I'm missing the point.

You said that,

  > It needs to:
  > 
  > 1. Get a device ref count when it remembers a device: (ie addif)
  > 2. Install a notifier that looks for device removal events
  > 3. In notifier, remove interface, including flushing all pending
  >    skb's for that device.

For 3, does the starter patch flush all pending skbs?  Perhaps you
could elaborate on what you had in mind?

I am trying to find the best way for the aoe driver to handle the
situation where ther are no usable local network interfaces.  It does
seem to me that the user would benefit from a notice letting them know
that they're trying to do AoE without any usable ethernet.  I'm
thinking that doing something like a printk_once would be appropriate.

(But probably not printk_once itself, because if they add a local
interface and then it goes away later, they should be told again that
something's wrong.)

-- 
  Ed Cashin <ecashin@...aid.com>
  http://www.coraid.com/
  http://noserose.net/e/
--
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