netdev - Re: [PATCH net-next v4 1/6] bonding: simplify and use RCU protection for 3ad xmit path

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: <522B3BF1.2020208@redhat.com>
Date:	Sat, 07 Sep 2013 16:45:05 +0200
From:	Nikolay Aleksandrov <nikolay@...hat.com>
To:	Veaceslav Falico <vfalico@...hat.com>
CC:	Ding Tianhong <dingtianhong@...wei.com>,
	Jay Vosburgh <fubar@...ibm.com>,
	Andy Gospodarek <andy@...yhouse.net>,
	"David S. Miller" <davem@...emloft.net>,
	Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v4 1/6] bonding: simplify and use RCU protection
 for 3ad xmit path


On 09/07/2013 04:20 PM, Veaceslav Falico wrote:
> On Fri, Sep 06, 2013 at 03:28:07PM +0800, Ding Tianhong wrote:
<snip>
> (Also, mind the "likely(__ptr != __next)" - usually, and bonding is a very
> good example, we always have at least one element in the list.)
> 
> This way, I'd recommend you to do bond_last_slave_rcu() the same way as
> here - you, though, might omit saving the slave_list pointer, it's not
> needed in case of bonding AFAIK. Something like that (I'm writing it in my
> mail editor - so it's only for the reference):
> 
> #define bond_last_slave_rcu(bond) \
>     ({struct list_head *__slave_ptr = list_next_rcu(&bond->slave_list); \
>       likely(__slave_ptr != &bond->slave_list) ? \
>         bond_to_slave_rcu(__slave_ptr) : NULL;})
>
> 
> Or, even better (from my POV), add a generic macro to rculist.h (again,
> didn't even compile it) - it can be used later on:
> 
> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> index f4b1001..37b49d1 100644
> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -23,6 +23,7 @@
>   * way, we must not access it directly
>   */
>  #define list_next_rcu(list)    (*((struct list_head __rcu
> **)(&(list)->next)))
> +#define list_prev_rcu(list)    (*((struct list_head __rcu
> **)(&(list)->prev)))
>  
>  /*
>   * Insert a new entry between two known consecutive entries.
> @@ -271,6 +272,12 @@ static inline void list_splice_init_rcu(struct
> list_head *list,
>        likely(__ptr != __next) ? container_of(__next, type, member) : NULL; \
>      })
>  
> +#define list_last_or_null_rcu(ptr, type, member) \
> +    ({struct list_head *__ptr = (ptr); \
> +      struct list_head __rcu *__last = list_prev_rcu(__ptr); \
> +      likely(__ptr != __last) ? container_of(__prev, type, member) : NULL; \
> +    })
> +
Hi,
Actually I don't think you can dereference ->prev and use the standard
list_del_rcu because it guarantees only the ->next ptr will be valid and
->prev is set to LIST_POISON2.
IMO, you'll need something like this: https://lkml.org/lkml/2012/7/25/193
with the bidir_del and all that.

But in any case I complete agree with Veaceslav here. Read all the
documentation carefully :-)

Cheers,
 Nik

>  /**
>   * list_for_each_entry_rcu    -    iterate over rcu list of given type
>   * @pos:    the type * to use as a loop cursor.
> ------- END OF PATCH ------
> 
> Anyway, it's up to you.
> 
> Hope that helps.

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