| 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
| ||
|
Message-ID: <20100527145710.GB6916@psychotron.lab.eng.brq.redhat.com>
Date: Thu, 27 May 2010 16:57:11 +0200
From: Jiri Pirko <jpirko@...hat.com>
To: Patrick McHardy <kaber@...sh.net>
Cc: netdev@...r.kernel.org, davem@...emloft.net, shemminger@...tta.com,
eric.dumazet@...il.com
Subject: Re: [PATCH net-next-2.6] net: replace hooks in __netif_receive_skb
Thu, May 27, 2010 at 04:06:54PM CEST, kaber@...sh.net wrote:
>Jiri Pirko wrote:
>> @@ -511,10 +512,16 @@ static void macvlan_setup(struct net_device *dev)
>> dev->tx_queue_len = 0;
>> }
>>
>> +static struct netdev_rx_handler macvlan_rx_handler = {
>> + .order = NETDEV_RX_HANDLER_ORDER_MACVLAN,
>> + .callback = macvlan_handle_frame,
>> +};
>
>It seems this could be const since you duplicate it on
>registration.
Noted.
>
>> +
>> static int macvlan_port_create(struct net_device *dev)
>> {
>> struct macvlan_port *port;
>> unsigned int i;
>> + int err;
>>
>> if (dev->type != ARPHRD_ETHER || dev->flags & IFF_LOOPBACK)
>> return -EINVAL;
>> @@ -528,6 +535,15 @@ static int macvlan_port_create(struct net_device *dev)
>> for (i = 0; i < MACVLAN_HASH_SIZE; i++)
>> INIT_HLIST_HEAD(&port->vlan_hash[i]);
>> rcu_assign_pointer(dev->macvlan_port, port);
>> +
>> + err = netdev_rx_handler_register(dev, &macvlan_rx_handler);
>> + if (err) {
>> + rcu_assign_pointer(dev->macvlan_port, NULL);
>> + synchronize_rcu();
>> + kfree(port);
>> + return err;
>> + }
>
>I'd prefer goto-based unroll since that makes changes in the
>future easier.
Ok, I thought this won't be necessary in this case, but right, looks better.
>
>> +
>> return 0;
>> }
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index a1bff65..8e95b2d 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -254,6 +254,15 @@ struct netdev_hw_addr_list {
>> #define netdev_for_each_mc_addr(ha, dev) \
>> netdev_hw_addr_list_for_each(ha, &(dev)->mc)
>>
>> +
>> +struct netdev_rx_handler {
>> + struct list_head list;
>> + unsigned int order;
>> +#define NETDEV_RX_HANDLER_ORDER_BRIDGE 1
>> +#define NETDEV_RX_HANDLER_ORDER_MACVLAN 2
>
>Any reason for not using an enum?
No, I can use enum, but this "inlining" gives a person who is looking at the
code the connection on the first look.
>
>> + struct sk_buff *(*callback)(struct sk_buff *skb);
>> +};
>> +
>> struct hh_cache {
>> struct hh_cache *hh_next; /* Next entry */
>> atomic_t hh_refcnt; /* number of users */
>> @@ -1031,6 +1040,10 @@ struct net_device {
>> /* GARP */
>> struct garp_port *garp_port;
>>
>> + /* receive handlers (hooks) list */
>> + spinlock_t rx_handlers_lock;
>> + struct list_head rx_handlers;
>> +
>> /* class/net/name entry */
>> struct device dev;
>> /* space for optional device, statistics, and wireless sysfs groups */
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 6c82065..8d4a817 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -2744,6 +2688,82 @@ void netif_nit_deliver(struct sk_buff *skb)
>> rcu_read_unlock();
>> }
>>
>> +static bool rx_handlers_equal(struct netdev_rx_handler *rh1,
>> + struct netdev_rx_handler *rh2)
>> +{
>> + return (rh1->order == rh2->order) && (rh1->callback == rh2->callback);
>> +}
>> +
>> +/**
>> + * netdev_rx_handler_register - register receive handler
>> + * @dev: device to register a handler for
>> + * @rh: receive handler to register
>> + *
>> + * Register a receive hander for a device. This handler will then be
>> + * called from __netif_receive_skb. A negative errno code is returned
>> + * on a failure.
>> + */
>> +int netdev_rx_handler_register(struct net_device *dev,
>> + struct netdev_rx_handler *rh)
>> +{
>> + struct list_head *list, *add_after;
>> + struct netdev_rx_handler *rh1;
>> + int err = 0;
>> +
>> + spin_lock_bh(&dev->rx_handlers_lock);
>
>Why are you using a spin lock and even disable BHs? This function
>should only be called from user context, so a mutex will work fine
>(and would fix the use of GFP_KERNEL in an atomic section).
Right, I will use rather rtnl_lock as suggested by Stephen.
>
>> + add_after = &dev->rx_handlers;
>> + list_for_each(list, &dev->rx_handlers) {
>
>Naming the element "list" is confusing. Also this should be
>using list_for_each_entry().
Well I'm not using list_for_each_entry because I use the list_head cursor as a
head in list_add_rcu (add_after assignment)
>
>> + rh1 = list_entry(list, struct netdev_rx_handler, list);
>> + if (rx_handlers_equal(rh, rh1)) {
>> + err = -EEXIST;
>> + goto unlock;
>> + }
>> + if (rh1->order > rh->order)
>> + break;
>> + add_after = list;
>> + }
>> + rh1 = kzalloc(sizeof(*rh), GFP_KERNEL);
>> + if (!rh1) {
>> + err = -ENOMEM;
>> + goto unlock;
>> + }
>> +
>> + rh1->order = rh->order;
>> + rh1->callback = rh->callback;
>> + list_add_rcu(&rh1->list, add_after);
>> +
>> +unlock:
>> + spin_unlock_bh(&dev->rx_handlers_lock);
>> +
>> + return err;
>> +}
>> +EXPORT_SYMBOL(netdev_rx_handler_register);
Thanks all for comments, I'll send V2 soon
--
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
Powered by blists - more mailing lists