Received-SPF: pass (google.com: domain of linux-kernel+bounces-63466-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 13 Feb 2024 12:53:04 +0100
From: Samuel Thibault <samuel.thibault@ens-lyon.org>
To: Tom Parkin <tparkin@katalix.com>
Cc: James Chapman <jchapman@katalix.com>, edumazet@google.com,
	gnault@redhat.com, davem@davemloft.net, kuba@kernel.org,
	pabeni@redhat.com, corbet@lwn.net, netdev@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCHv4] PPPoL2TP: Add more code snippets
Message-ID: <20240213115304.3oyqkvkb3oqkauwd@begin>
Mail-Followup-To: Samuel Thibault <samuel.thibault@ens-lyon.org>,
	Tom Parkin <tparkin@katalix.com>,
	James Chapman <jchapman@katalix.com>, edumazet@google.com,
	gnault@redhat.com, davem@davemloft.net, kuba@kernel.org,
	pabeni@redhat.com, corbet@lwn.net, netdev@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20240212222344.xtv233r5sixme32h@begin>
 <ZctJnCeUCANJvxGj@katalix.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ZctJnCeUCANJvxGj@katalix.com>
Organization: I am not organized
User-Agent: NeoMutt/20170609 (1.8.3)

Tom Parkin, le mar. 13 févr. 2024 10:51:08 +0000, a ecrit:
> > +        ret = ioctl(session_fd1, PPPIOCGCHAN, &chindx1);
> > +        if (ret < 0)
> > +                return -errno;
> > +
> > +        ret = ioctl(session_fd2, PPPIOCGCHAN, &chindx2);
> > +        if (ret < 0)
> > +                return -errno;
> > +
> > +        ppp_chan_fd = open("/dev/ppp", O_RDWR);
> > +        if (ppp_chan_fd < 0) {
> > +                return -errno;
> > +        }
> > +
> > +        ret = ioctl(ppp_chan_fd, PPPIOCATTCHAN, &chindx1);
> > +        if (ret < 0) {
> > +                close(ppp_chan_fd);
> > +                return -errno;
> > +        }
> 
> I think we should drop the PPPIOCATTCHAN ioctl call here.
> 
> The input file descriptors are called out as being PPPoX sockets
> created as described earlier, in which case they should both
> already be attached to a channel.
> 
> It would make more sense IMO to call out the two ppp_chan_fd file
> descriptors as being input parameters alongside the PPPoX session file
> descriptors.
> 
> > +
> > +        ret = ioctl(ppp_chan_fd, PPPIOCBRIDGECHAN, &chindx2);
> > +        close(ppp_chan_fd);
> > +        if (ret < 0)
> > +                return -errno;
> > +
> > +It can be noted that in this case no PPP interface is needed, and the PPP
> > +channel does not need to be kept open.  Only the session PPPoX data sockets need
> > +to be kept open.
> 
> Is it true to say that the PPP channel file descriptors can be closed
> by userspace?

In our code we do it
https://code.ffdn.org/sthibaul/l2tpns/-/blob/kernel/l2tpns.c?ref_type=heads#L1295
and it works all fine indeed (and avoids that fd per session).

That's actually one of the reason why I made the snipped only take the
pppox sockets, and make it create the ppp chan fd only temporarily. AIUI
the pppox socket already has a ppp chan (returned by PPPIOCGCHAN), and
the ppp chan fd is there only for performing the bridging ioctl.

Samuel