Re: [PATCH net] l2tp: Allow duplicate session creation with UDP

[Guillaume Nault <gnault@redhat.com>]

On Fri, Jan 17, 2020 at 10:26:09AM +1300, Ridge Kennedy wrote:
> On Thu, 16 Jan 2020, Guillaume Nault wrote:
> > On Thu, Jan 16, 2020 at 11:34:47AM +1300, Ridge Kennedy wrote:
> > > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> > > index f82ea12bac37..0cc86227c618 100644
> > > --- a/net/l2tp/l2tp_core.c
> > > +++ b/net/l2tp/l2tp_core.c
> > > @@ -323,7 +323,9 @@ int l2tp_session_register(struct l2tp_session *session,
> > >  		spin_lock_bh(&pn->l2tp_session_hlist_lock);
> > > 
> > >  		hlist_for_each_entry(session_walk, g_head, global_hlist)
> > > -			if (session_walk->session_id == session->session_id) {
> > > +			if (session_walk->session_id == session->session_id &&
> > > +			    (session_walk->tunnel->encap == L2TP_ENCAPTYPE_IP ||
> > > +			     tunnel->encap == L2TP_ENCAPTYPE_IP)) {
> > >  				err = -EEXIST;
> > >  				goto err_tlock_pnlock;
> > >  			}
> > Well, I think we have a more fundamental problem here. By adding
> > L2TPoUDP sessions to the global list, we allow cross-talks with L2TPoIP
> > sessions. That is, if we have an L2TPv3 session X running over UDP and
> > we receive an L2TP_IP packet targetted at session ID X, then
> > l2tp_session_get() will return the L2TP_UDP session to l2tp_ip_recv().
> > 
> > I guess l2tp_session_get() should be dropped and l2tp_ip_recv() should
> > look up the session in the context of its socket, like in the UDP case.
> > 
> > But for the moment, what about just not adding L2TP_UDP sessions to the
> > global list? That should fix both your problem and the L2TP_UDP/L2TP_IP
> > cross-talks.
> 
> I did consider not adding the L2TP_UDP sessions to the global list, but that
> change looked a little more involved as the netlink interface was also
> making use of the list to lookup sessions by ifname. At a minimum
> it looks like this would require rework of l2tp_session_get_by_ifname().
> 
Yes, you're right. Now that we all agree on this fix, can you please
repost your patch?

BTW, I wouldn't mind a small comment on top of the conditional to
explain that IP encap assumes globally unique session IDs while UDP
doesn't.