netdev - Re: [PATCH iwl-next v3 3/6] pfcp: add PFCP module

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: <ZLqcDf68HgB6Knnk@smile.fi.intel.com>
Date: Fri, 21 Jul 2023 17:54:05 +0300
From: Andy Shevchenko <andy@...nel.org>
To: Marcin Szycik <marcin.szycik@...ux.intel.com>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
	wojciech.drewek@...el.com, michal.swiatkowski@...ux.intel.com,
	aleksander.lobakin@...el.com, davem@...emloft.net, kuba@...nel.org,
	jiri@...nulli.us, pabeni@...hat.com, jesse.brandeburg@...el.com,
	simon.horman@...igine.com, idosch@...dia.com
Subject: Re: [PATCH iwl-next v3 3/6] pfcp: add PFCP module

On Fri, Jul 21, 2023 at 09:15:29AM +0200, Marcin Szycik wrote:
> From: Wojciech Drewek <wojciech.drewek@...el.com>
> 
> Packet Forwarding Control Protocol (PFCP) is a 3GPP Protocol
> used between the control plane and the user plane function.
> It is specified in TS 29.244[1].
> 
> Note that this module is not designed to support this Protocol
> in the kernel space. There is no support for parsing any PFCP messages.
> There is no API that could be used by any userspace daemon.
> Basically it does not support PFCP. This protocol is sophisticated
> and there is no need for implementing it in the kernel. The purpose
> of this module is to allow users to setup software and hardware offload
> of PFCP packets using tc tool.
> 
> When user requests to create a PFCP device, a new socket is created.
> The socket is set up with port number 8805 which is specific for
> PFCP [29.244 4.2.2]. This allow to receive PFCP request messages,
> response messages use other ports.
> 
> Note that only one PFCP netdev can be created.
> 
> Only IPv4 is supported at this time.
> 
> [1] https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=3111

> Signed-off-by: Wojciech Drewek <wojciech.drewek@...el.com>

Co-developed-by: Marcin...?

> Signed-off-by: Marcin Szycik <marcin.szycik@...ux.intel.com>

...

> +/* PFCP according to 3GPP TS 29.244
> + *
> + * Copyright (C) 2022, Intel Corporation.

> + * (C) 2022 by Wojciech Drewek <wojciech.drewek@...el.com>

Is it approved by our Legal? First time I see such (c) together with Intel's
and correct authorship.

> + * Author: Wojciech Drewek <wojciech.drewek@...el.com>
> + */

...

> +struct pfcp_dev {
> +	struct list_head	list;

This is defined in types.h which is missing.

> +	struct socket		*sock;
> +	struct net_device	*dev;
> +	struct net		*net;
> +};

...

> +	dev->needs_free_netdev	= true;

Single space is enough.

...

> +static int pfcp_newlink(struct net *net, struct net_device *dev,
> +			struct nlattr *tb[], struct nlattr *data[],
> +			struct netlink_ext_ack *extack)
> +{
> +	struct pfcp_dev *pfcp = netdev_priv(dev);
> +	struct pfcp_net *pn;
> +	int err;
> +
> +	pfcp->net = net;
> +
> +	err = pfcp_add_sock(pfcp);
> +	if (err) {
> +		netdev_dbg(dev, "failed to add pfcp socket %d\n", err);
> +		goto exit;
> +	}
> +
> +	err = register_netdevice(dev);
> +	if (err) {
> +		netdev_dbg(dev, "failed to register pfcp netdev %d\n", err);
> +		goto exit_reg_netdev;
> +	}
> +
> +	pn = net_generic(dev_net(dev), pfcp_net_id);
> +	list_add_rcu(&pfcp->list, &pn->pfcp_dev_list);
> +
> +	netdev_dbg(dev, "registered new PFCP interface\n");
> +
> +	return 0;
> +
> +exit_reg_netdev:

The label naming should tell what _will_ happen if goto $LABEL.
Something like

exit_del_pfcp_sock:

Ditto for all labels in your code.

> +	pfcp_del_sock(pfcp);
> +exit:

Shouldn't here be

	->net = NULL;

?

> +	return err;
> +}

...

> +#ifndef _PFCP_H_
> +#define _PFCP_H_

Missing headers:
For net_device internals, bool type, and strcpm() call.

> +#define PFCP_PORT 8805
> +
> +static inline bool netif_is_pfcp(const struct net_device *dev)
> +{
> +	return dev->rtnl_link_ops &&
> +	       !strcmp(dev->rtnl_link_ops->kind, "pfcp");
> +}
> +
> +#endif

-- 
With Best Regards,
Andy Shevchenko