lkml.org 
[lkml]   [2017]   [Nov]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    Subject[PATCH 3.16 157/294] l2tp: hold tunnel while looking up sessions in l2tp_netlink
    3.16.50-rc1 review patch.  If anyone has any objections, please let me know.

    ------------------

    From: Guillaume Nault <g.nault@alphalink.fr>

    commit 54652eb12c1b72e9602d09cb2821d5760939190f upstream.

    l2tp_tunnel_find() doesn't take a reference on the returned tunnel.
    Therefore, it's unsafe to use it because the returned tunnel can go
    away on us anytime.

    Fix this by defining l2tp_tunnel_get(), which works like
    l2tp_tunnel_find(), but takes a reference on the returned tunnel.
    Caller then has to drop this reference using l2tp_tunnel_dec_refcount().

    As l2tp_tunnel_dec_refcount() needs to be moved to l2tp_core.h, let's
    simplify the patch and not move the L2TP_REFCNT_DEBUG part. This code
    has been broken (not even compiling) in May 2012 by
    commit a4ca44fa578c ("net: l2tp: Standardize logging styles")
    and fixed more than two years later by
    commit 29abe2fda54f ("l2tp: fix missing line continuation"). So it
    doesn't appear to be used by anyone.

    Same thing for l2tp_tunnel_free(); instead of moving it to l2tp_core.h,
    let's just simplify things and call kfree_rcu() directly in
    l2tp_tunnel_dec_refcount(). Extra assertions and debugging code
    provided by l2tp_tunnel_free() didn't help catching any of the
    reference counting and socket handling issues found while working on
    this series.

    Fixes: 309795f4bec2 ("l2tp: Add netlink control API for L2TP")
    Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
    Signed-off-by: David S. Miller <davem@davemloft.net>
    [bwh: Backported to 3.16: keep using atomic_t functions]
    Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
    ---
    net/l2tp/l2tp_core.c | 66 ++++++++++++++++---------------------------------
    net/l2tp/l2tp_core.h | 13 ++++++++++
    net/l2tp/l2tp_netlink.c | 6 +++--
    3 files changed, 38 insertions(+), 47 deletions(-)

    --- a/net/l2tp/l2tp_core.c
    +++ b/net/l2tp/l2tp_core.c
    @@ -112,7 +112,6 @@ struct l2tp_net {
    spinlock_t l2tp_session_hlist_lock;
    };

    -static void l2tp_tunnel_free(struct l2tp_tunnel *tunnel);

    static inline struct l2tp_tunnel *l2tp_tunnel(struct sock *sk)
    {
    @@ -126,39 +125,6 @@ static inline struct l2tp_net *l2tp_pern
    return net_generic(net, l2tp_net_id);
    }

    -/* Tunnel reference counts. Incremented per session that is added to
    - * the tunnel.
    - */
    -static inline void l2tp_tunnel_inc_refcount_1(struct l2tp_tunnel *tunnel)
    -{
    - atomic_inc(&tunnel->ref_count);
    -}
    -
    -static inline void l2tp_tunnel_dec_refcount_1(struct l2tp_tunnel *tunnel)
    -{
    - if (atomic_dec_and_test(&tunnel->ref_count))
    - l2tp_tunnel_free(tunnel);
    -}
    -#ifdef L2TP_REFCNT_DEBUG
    -#define l2tp_tunnel_inc_refcount(_t) \
    -do { \
    - pr_debug("l2tp_tunnel_inc_refcount: %s:%d %s: cnt=%d\n", \
    - __func__, __LINE__, (_t)->name, \
    - atomic_read(&_t->ref_count)); \
    - l2tp_tunnel_inc_refcount_1(_t); \
    -} while (0)
    -#define l2tp_tunnel_dec_refcount(_t)
    -do { \
    - pr_debug("l2tp_tunnel_dec_refcount: %s:%d %s: cnt=%d\n", \
    - __func__, __LINE__, (_t)->name, \
    - atomic_read(&_t->ref_count)); \
    - l2tp_tunnel_dec_refcount_1(_t); \
    -} while (0)
    -#else
    -#define l2tp_tunnel_inc_refcount(t) l2tp_tunnel_inc_refcount_1(t)
    -#define l2tp_tunnel_dec_refcount(t) l2tp_tunnel_dec_refcount_1(t)
    -#endif
    -
    /* Session hash global list for L2TPv3.
    * The session_id SHOULD be random according to RFC3931, but several
    * L2TP implementations use incrementing session_ids. So we do a real
    @@ -277,6 +243,27 @@ struct l2tp_session *l2tp_session_find(s
    }
    EXPORT_SYMBOL_GPL(l2tp_session_find);

    +/* Lookup a tunnel. A new reference is held on the returned tunnel. */
    +struct l2tp_tunnel *l2tp_tunnel_get(const struct net *net, u32 tunnel_id)
    +{
    + const struct l2tp_net *pn = l2tp_pernet(net);
    + struct l2tp_tunnel *tunnel;
    +
    + rcu_read_lock_bh();
    + list_for_each_entry_rcu(tunnel, &pn->l2tp_tunnel_list, list) {
    + if (tunnel->tunnel_id == tunnel_id) {
    + l2tp_tunnel_inc_refcount(tunnel);
    + rcu_read_unlock_bh();
    +
    + return tunnel;
    + }
    + }
    + rcu_read_unlock_bh();
    +
    + return NULL;
    +}
    +EXPORT_SYMBOL_GPL(l2tp_tunnel_get);
    +
    /* Like l2tp_session_find() but takes a reference on the returned session.
    * Optionally calls session->ref() too if do_ref is true.
    */
    @@ -1396,17 +1383,6 @@ static void l2tp_udp_encap_destroy(struc
    }
    }

    -/* Really kill the tunnel.
    - * Come here only when all sessions have been cleared from the tunnel.
    - */
    -static void l2tp_tunnel_free(struct l2tp_tunnel *tunnel)
    -{
    - BUG_ON(atomic_read(&tunnel->ref_count) != 0);
    - BUG_ON(tunnel->sock != NULL);
    - l2tp_info(tunnel, L2TP_MSG_CONTROL, "%s: free...\n", tunnel->name);
    - kfree_rcu(tunnel, rcu);
    -}
    -
    /* Workqueue tunnel deletion function */
    static void l2tp_tunnel_del_work(struct work_struct *work)
    {
    --- a/net/l2tp/l2tp_core.h
    +++ b/net/l2tp/l2tp_core.h
    @@ -240,6 +240,8 @@ out:
    return tunnel;
    }

    +struct l2tp_tunnel *l2tp_tunnel_get(const struct net *net, u32 tunnel_id);
    +
    struct l2tp_session *l2tp_session_get(const struct net *net,
    struct l2tp_tunnel *tunnel,
    u32 session_id, bool do_ref);
    @@ -281,6 +283,17 @@ int l2tp_nl_register_ops(enum l2tp_pwtyp
    void l2tp_nl_unregister_ops(enum l2tp_pwtype pw_type);
    int l2tp_ioctl(struct sock *sk, int cmd, unsigned long arg);

    +static inline void l2tp_tunnel_inc_refcount(struct l2tp_tunnel *tunnel)
    +{
    + atomic_inc(&tunnel->ref_count);
    +}
    +
    +static inline void l2tp_tunnel_dec_refcount(struct l2tp_tunnel *tunnel)
    +{
    + if (atomic_dec_and_test(&tunnel->ref_count))
    + kfree_rcu(tunnel, rcu);
    +}
    +
    /* Session reference counts. Incremented when code obtains a reference
    * to a session.
    */
    --- a/net/l2tp/l2tp_netlink.c
    +++ b/net/l2tp/l2tp_netlink.c
    @@ -60,10 +60,12 @@ static struct l2tp_session *l2tp_nl_sess
    (info->attrs[L2TP_ATTR_CONN_ID])) {
    tunnel_id = nla_get_u32(info->attrs[L2TP_ATTR_CONN_ID]);
    session_id = nla_get_u32(info->attrs[L2TP_ATTR_SESSION_ID]);
    - tunnel = l2tp_tunnel_find(net, tunnel_id);
    - if (tunnel)
    + tunnel = l2tp_tunnel_get(net, tunnel_id);
    + if (tunnel) {
    session = l2tp_session_get(net, tunnel, session_id,
    do_ref);
    + l2tp_tunnel_dec_refcount(tunnel);
    + }
    }

    return session;
    \
     
     \ /
      Last update: 2017-11-08 20:58    [W:4.880 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site