Received-SPF: pass (google.com: domain of linux-kernel+bounces-63544-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 13 Feb 2024 12:58:40 +0000
From: Tom Parkin <tparkin@katalix.com>
To: Samuel Thibault <samuel.thibault@ens-lyon.org>,
	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: <ZctngNzLYe/+Iman@katalix.com>
References: <20240212222344.xtv233r5sixme32h@begin>
 <ZctJnCeUCANJvxGj@katalix.com>
 <20240213115304.3oyqkvkb3oqkauwd@begin>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="G2qaJ+yw4/XCnyb6"
Content-Disposition: inline
In-Reply-To: <20240213115304.3oyqkvkb3oqkauwd@begin>


--G2qaJ+yw4/XCnyb6
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On  Tue, Feb 13, 2024 at 12:53:04 +0100, Samuel Thibault wrote:
> Tom Parkin, le mar. 13 f=E9vr. 2024 10:51:08 +0000, a ecrit:
> > > +        ret =3D ioctl(session_fd1, PPPIOCGCHAN, &chindx1);
> > > +        if (ret < 0)
> > > +                return -errno;
> > > +
> > > +        ret =3D ioctl(session_fd2, PPPIOCGCHAN, &chindx2);
> > > +        if (ret < 0)
> > > +                return -errno;
> > > +
> > > +        ppp_chan_fd =3D open("/dev/ppp", O_RDWR);
> > > +        if (ppp_chan_fd < 0) {
> > > +                return -errno;
> > > +        }
> > > +
> > > +        ret =3D ioctl(ppp_chan_fd, PPPIOCATTCHAN, &chindx1);
> > > +        if (ret < 0) {
> > > +                close(ppp_chan_fd);
> > > +                return -errno;
> > > +        }
> >=20
> > I think we should drop the PPPIOCATTCHAN ioctl call here.
> >=20
> > 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.
> >=20
> > 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.
> >=20
> > > +
> > > +        ret =3D 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 th=
e PPP
> > > +channel does not need to be kept open.  Only the session PPPoX data =
sockets need
> > > +to be kept open.
> >=20
> > Is it true to say that the PPP channel file descriptors can be closed
> > by userspace?
>=20
> In our code we do it
> https://code.ffdn.org/sthibaul/l2tpns/-/blob/kernel/l2tpns.c?ref_type=3Dh=
eads#L1295
> and it works all fine indeed (and avoids that fd per session).
>=20
> 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.

Thanks for the code reference -- that makes it clearer.  And I'm glad
someone (else) is using PPPIOCBRIDGECHAN :-)

It's a while since I was looking in ppp_generic.c and you're right
about the ppp channel fd.
--=20
Tom Parkin
Katalix Systems Ltd
https://katalix.com
Catalysts for your Embedded Linux software development

--G2qaJ+yw4/XCnyb6
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEsUkgyDzMwrj81nq0lIwGZQq6i9AFAmXLZ30ACgkQlIwGZQq6
i9DYkAf9GOhgqPlIofeqsI0xOlcmwUwi/8tE99f4HV85ghT242yIB66IYVCyGijG
L5llsSOJ+d3cjw1/FrDAOFWuPira89FijTidj5ZgXApkGpHZo6Uomq/jYjNLXMh/
cWqbMo53t2EMrwUgl804vkGAXQ5H9mIkuDGMegjO/Ld3pTDWstwwQWcZZBmUb33f
116FyZcPNDzXmLvSDi7EBQBoXdZOXcXEaJl9XZZXgGjWrBzcKvs2Ed/emRKUUGRI
ItlC1udf1eJ4RijloRzOUykwwigd/DdNEqb2xocrhQPnhFLBc2/Qj1iHYJYsMj24
+U6BGaqqzeVjSqLucUAjj0zKFsOeOg==
=+am9
-----END PGP SIGNATURE-----

--G2qaJ+yw4/XCnyb6--