[PATCH net-next] macvlan: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to real device

[PATCH net-next] macvlan: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to real device

[Hangbin Liu]

Similiar to commit a6111d3c93d0 ("vlan: Pass SIOC[SG]HWTSTAMP ioctls to
real device") and commit 37dd9255b2f6 ("vlan: Pass ethtool get_ts_info
queries to real device."), add MACVlan HW ptp support.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/macvlan.c | 48 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 0c0f105657d3..4a6be8fab884 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -24,6 +24,7 @@
 #include <linux/notifier.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
+#include <linux/net_tstamp.h>
 #include <linux/ethtool.h>
 #include <linux/if_arp.h>
 #include <linux/if_vlan.h>
@@ -34,6 +35,7 @@
 #include <net/rtnetlink.h>
 #include <net/xfrm.h>
 #include <linux/netpoll.h>
+#include <linux/phy.h>
 
 #define MACVLAN_HASH_BITS	8
 #define MACVLAN_HASH_SIZE	(1<<MACVLAN_HASH_BITS)
@@ -822,6 +824,30 @@ static int macvlan_change_mtu(struct net_device *dev, int new_mtu)
 	return 0;
 }
 
+static int macvlan_do_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
+{
+	struct net_device *real_dev = macvlan_dev_real_dev(dev);
+	const struct net_device_ops *ops = real_dev->netdev_ops;
+	struct ifreq ifrr;
+	int err = -EOPNOTSUPP;
+
+	strncpy(ifrr.ifr_name, real_dev->name, IFNAMSIZ);
+	ifrr.ifr_ifru = ifr->ifr_ifru;
+
+	switch (cmd) {
+	case SIOCSHWTSTAMP:
+	case SIOCGHWTSTAMP:
+		if (netif_device_present(real_dev) && ops->ndo_do_ioctl)
+			err = ops->ndo_do_ioctl(real_dev, &ifrr, cmd);
+		break;
+	}
+
+	if (!err)
+		ifr->ifr_ifru = ifrr.ifr_ifru;
+
+	return err;
+}
+
 /*
  * macvlan network devices have devices nesting below it and are a special
  * "super class" of normal network devices; split their locks off into a
@@ -1020,6 +1046,26 @@ static int macvlan_ethtool_get_link_ksettings(struct net_device *dev,
 	return __ethtool_get_link_ksettings(vlan->lowerdev, cmd);
 }
 
+static int macvlan_ethtool_get_ts_info(struct net_device *dev,
+				       struct ethtool_ts_info *info)
+{
+	struct net_device *real_dev = macvlan_dev_real_dev(dev);
+	const struct ethtool_ops *ops = real_dev->ethtool_ops;
+	struct phy_device *phydev = real_dev->phydev;
+
+	if (phydev && phydev->drv && phydev->drv->ts_info) {
+		 return phydev->drv->ts_info(phydev, info);
+	} else if (ops->get_ts_info) {
+		return ops->get_ts_info(real_dev, info);
+	} else {
+		info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
+			SOF_TIMESTAMPING_SOFTWARE;
+		info->phc_index = -1;
+	}
+
+	return 0;
+}
+
 static netdev_features_t macvlan_fix_features(struct net_device *dev,
 					      netdev_features_t features)
 {
@@ -1094,6 +1140,7 @@ static const struct ethtool_ops macvlan_ethtool_ops = {
 	.get_link		= ethtool_op_get_link,
 	.get_link_ksettings	= macvlan_ethtool_get_link_ksettings,
 	.get_drvinfo		= macvlan_ethtool_get_drvinfo,
+	.get_ts_info		= macvlan_ethtool_get_ts_info,
 };
 
 static const struct net_device_ops macvlan_netdev_ops = {
@@ -1103,6 +1150,7 @@ static const struct net_device_ops macvlan_netdev_ops = {
 	.ndo_stop		= macvlan_stop,
 	.ndo_start_xmit		= macvlan_start_xmit,
 	.ndo_change_mtu		= macvlan_change_mtu,
+	.ndo_do_ioctl		= macvlan_do_ioctl,
 	.ndo_fix_features	= macvlan_fix_features,
 	.ndo_change_rx_flags	= macvlan_change_rx_flags,
 	.ndo_set_mac_address	= macvlan_set_mac_address,
-- 
2.19.2

Re: [PATCH net-next] macvlan: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to real device

[David Miller]

From: Hangbin Liu <liuhangbin@gmail.com>
Date: Wed, 20 Mar 2019 10:23:33 +0800

> Similiar to commit a6111d3c93d0 ("vlan: Pass SIOC[SG]HWTSTAMP ioctls to
> real device") and commit 37dd9255b2f6 ("vlan: Pass ethtool get_ts_info
> queries to real device."), add MACVlan HW ptp support.
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Applied.

Re: [PATCH net-next] macvlan: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to real device

[Hangbin Liu]

On Wed, Mar 20, 2019 at 10:23:33AM +0800, Hangbin Liu wrote:
> Similiar to commit a6111d3c93d0 ("vlan: Pass SIOC[SG]HWTSTAMP ioctls to
> real device") and commit 37dd9255b2f6 ("vlan: Pass ethtool get_ts_info
> queries to real device."), add MACVlan HW ptp support.
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Hi all,

For this patch. Jiri mentioned that running two ptp4l/phc2sys instances on two
containers will not work. But I think the admin should avoid this scenario as
we should not run two phc2sys instances on the same real device. In NET_ADMIN
enabled containers, if a normal user which has mapped to root wants to run
ptp4l, the admin need either add the device specifically (--device=/dev/ptp1)
or give the container privileged permission (--privileged). So I think this
should not be a security problem.

On the other hand, Miroslav pointed that with NET_ADMIN enabled in container,
a normal user could be mapped to root and is able to change the real devices's
rx filter via ioctl on macvlan, which may affect the other ptp process on
host. ptp over vlan also has this issue, but macvlan is more frequently used
in container. To prevent this issue, here are some options:

1. limit ioctl SIOCSHWTSTAMP to only enabling timestamping and switching to
   more general filters. The limitation could be only in container and leave
   init_net free.
2. reject SIOCSHWTSTAMP in container.
3. revert the patch. The vlan part is too late to revert.

So what do you think? which one do you prefer?

Thanks
Hangbin

Re: [PATCH net-next] macvlan: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to real device

[Richard Cochran]

On Wed, Apr 17, 2019 at 04:05:09PM +0800, Hangbin Liu wrote:
> On the other hand, Miroslav pointed that with NET_ADMIN enabled in container,
> a normal user could be mapped to root and is able to change the real devices's
> rx filter via ioctl on macvlan, which may affect the other ptp process on
> host. ptp over vlan also has this issue, but macvlan is more frequently used
> in container.

If NET_ADMIN is enabled in the container, don't the host and container
contend with each other for the physical interfaces anyhow?

(I'm not a container person, so forgive my ignorance.)

Thanks,
Richard

Re: [PATCH net-next] macvlan: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to real device

[Jiri Benc]

On Wed, 17 Apr 2019 08:43:06 -0700, Richard Cochran wrote:
> If NET_ADMIN is enabled in the container, don't the host and container
> contend with each other for the physical interfaces anyhow?

Physical interfaces are not a problem, as each interface can be only in
a single net name space.

The problem here is this patch gives access to physical interface
settings through a virtual interface layered on top of it. Whenever
such thing is done, the virtual interface needs to provide a suitable
way of moderating access to the shared resources, so the individual
virtual interfaces do not affect each other. That's not what's being
done here.

I think this patch is wrong.

 Jiri

Re: [PATCH net-next] macvlan: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to real device

[Richard Cochran]

On Wed, Apr 17, 2019 at 08:59:58PM +0200, Jiri Benc wrote:
> The problem here is this patch gives access to physical interface
> settings through a virtual interface layered on top of it. Whenever
> such thing is done, the virtual interface needs to provide a suitable
> way of moderating access to the shared resources, so the individual
> virtual interfaces do not affect each other. That's not what's being
> done here.

So I guess the macvlan should reject SIOCSHWTSTAMP but allow
SIOCGHWTSTAMP.
 
> I think this patch is wrong.

But what about this statement:

    ptp over vlan also has this issue

What is the issue with VLAN interfaces?  Are these exportable to a
container when the physical interface is not?

Thanks,
Richard

Re: [PATCH net-next] macvlan: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to real device

[Hangbin Liu]

On Wed, Apr 17, 2019 at 08:31:57PM -0700, Richard Cochran wrote:
> On Wed, Apr 17, 2019 at 08:59:58PM +0200, Jiri Benc wrote:
> > The problem here is this patch gives access to physical interface
> > settings through a virtual interface layered on top of it. Whenever
> > such thing is done, the virtual interface needs to provide a suitable
> > way of moderating access to the shared resources, so the individual
> > virtual interfaces do not affect each other. That's not what's being
> > done here.
> 
> So I guess the macvlan should reject SIOCSHWTSTAMP but allow
> SIOCGHWTSTAMP.

Do you want to fix it only in container, like:

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 4a6be8fab884..a2bd50a50e2f 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -836,6 +836,8 @@ static int macvlan_do_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)

        switch (cmd) {
        case SIOCSHWTSTAMP:
+               if (!net_eq(dev_net(dev), &init_net))
+                       break;
        case SIOCGHWTSTAMP:
                if (netif_device_present(real_dev) && ops->ndo_do_ioctl)
                        err = ops->ndo_do_ioctl(real_dev, &ifrr, cmd);

Or just remove 'case SIOCSHWTSTAMP' directly?

And we should do the same on vlan, right?

>  
> > I think this patch is wrong.
> 
> But what about this statement:
> 
>     ptp over vlan also has this issue
> 
> What is the issue with VLAN interfaces?  Are these exportable to a
> container when the physical interface is not?

Yes, if we attach a VLAN interface to container, a mapped user could also
be able to modify the host's physical interface rx filter via ioctl.

Thanks
Hangbin

Re: [PATCH net-next] macvlan: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to real device

[Miroslav Lichvar]

On Wed, Apr 17, 2019 at 08:31:57PM -0700, Richard Cochran wrote:
> On Wed, Apr 17, 2019 at 08:59:58PM +0200, Jiri Benc wrote:
> > The problem here is this patch gives access to physical interface
> > settings through a virtual interface layered on top of it. Whenever
> > such thing is done, the virtual interface needs to provide a suitable
> > way of moderating access to the shared resources, so the individual
> > virtual interfaces do not affect each other. That's not what's being
> > done here.
> 
> So I guess the macvlan should reject SIOCSHWTSTAMP but allow
> SIOCGHWTSTAMP.

FWIW, my suggestion was to limit what the SIOCSHWTSTAMP ioctl can do
on the virtual interface. It could only enable HW timestamping or
select a more general filter. A container could run a PTP clock if it
had also access to the PHC device, or it could have the NET_ADMIN
capability for other reasons, but it couldn't disable HW timestamping
enabled by the host or other container.

If I understand it correctly, even without this ioctl a container can
prevent the host or other containers from getting some of the HW
timestamps by requesting TX timestamps at a high rate. I suspect the
timestamping would need to be restricted to the real interface to
fully protect it from applications having access to the virtual
interfaces.

-- 
Miroslav Lichvar

Re: [PATCH net-next] macvlan: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to real device

[Hangbin Liu]

Hi Miroslav,
On Thu, Apr 18, 2019 at 10:05:09AM +0200, Miroslav Lichvar wrote:
> > So I guess the macvlan should reject SIOCSHWTSTAMP but allow
> > SIOCGHWTSTAMP.
> 
> FWIW, my suggestion was to limit what the SIOCSHWTSTAMP ioctl can do
> on the virtual interface. It could only enable HW timestamping or

I think this is not enough as user could enable HWTSTAMP_FILTER_NONE.

> select a more general filter. A container could run a PTP clock if it

Do you have an idea about how to select a general filter? If we have enabled
HWTSTAMP_FILTER_PTP_V2_L4_SYNC on host and a user in container want to enable
HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ, then which one is more general?

> had also access to the PHC device, or it could have the NET_ADMIN
> capability for other reasons, but it couldn't disable HW timestamping
> enabled by the host or other container.
> 
> If I understand it correctly, even without this ioctl a container can
> prevent the host or other containers from getting some of the HW
> timestamps by requesting TX timestamps at a high rate. I suspect the

Could traffic sharping/limitation fix it?

> timestamping would need to be restricted to the real interface to
> fully protect it from applications having access to the virtual
> interfaces.

Thanks
Hangbin

Re: [PATCH net-next] macvlan: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to real device

[Miroslav Lichvar]

On Tue, Apr 23, 2019 at 12:18:17PM +0800, Hangbin Liu wrote:
> On Thu, Apr 18, 2019 at 10:05:09AM +0200, Miroslav Lichvar wrote:
> > select a more general filter. A container could run a PTP clock if it
> 
> Do you have an idea about how to select a general filter? If we have enabled
> HWTSTAMP_FILTER_PTP_V2_L4_SYNC on host and a user in container want to enable
> HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ, then which one is more general?

In this case neither is a more general filter of the other. If
V2_L4_SYNC is already selected, only the following filters could be
selected on the macvlan interface:

	HWTSTAMP_FILTER_PTP_V2_L4_SYNC,
	HWTSTAMP_FILTER_PTP_V2_L4_EVENT,
	HWTSTAMP_FILTER_PTP_V2_SYNC,
	HWTSTAMP_FILTER_PTP_V2_EVENT,
	HWTSTAMP_FILTER_ALL,

I think one way to check this would be to assign each filter a
(16-bit?) value where the individual bits correspond to the message
types and the newly selected filter would have to contain all bits of
the old one.

> > If I understand it correctly, even without this ioctl a container can
> > prevent the host or other containers from getting some of the HW
> > timestamps by requesting TX timestamps at a high rate. I suspect the
> 
> Could traffic sharping/limitation fix it?

Yes, but it has to be specific to packets with TX timestamp requested.
From what I have seen, TX timestamping may start to fail at just few
tens of thousands of packets per second.

-- 
Miroslav Lichvar

Re: [PATCH net-next] macvlan: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to real device

[Hangbin Liu]

On Tue, Apr 23, 2019 at 10:31:41AM +0200, Miroslav Lichvar wrote:
> On Tue, Apr 23, 2019 at 12:18:17PM +0800, Hangbin Liu wrote:
> > On Thu, Apr 18, 2019 at 10:05:09AM +0200, Miroslav Lichvar wrote:
> > > select a more general filter. A container could run a PTP clock if it
> > 
> > Do you have an idea about how to select a general filter? If we have enabled
> > HWTSTAMP_FILTER_PTP_V2_L4_SYNC on host and a user in container want to enable
> > HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ, then which one is more general?
> 
> In this case neither is a more general filter of the other. If

Yes, that what I mean, some times it's hard to say which one is more general.
like PTP_V2_L4_EVENT and PTP_V2_L2_SYNC.

> V2_L4_SYNC is already selected, only the following filters could be
> selected on the macvlan interface:
> 
> 	HWTSTAMP_FILTER_PTP_V2_L4_SYNC,
> 	HWTSTAMP_FILTER_PTP_V2_L4_EVENT,
> 	HWTSTAMP_FILTER_PTP_V2_SYNC,
> 	HWTSTAMP_FILTER_PTP_V2_EVENT,
> 	HWTSTAMP_FILTER_ALL,
> 
> I think one way to check this would be to assign each filter a
> (16-bit?) value where the individual bits correspond to the message
> types and the newly selected filter would have to contain all bits of
> the old one.

Just like I said, how to compare with different types.

Thanks
Hangbin

Re: [PATCH net-next] macvlan: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to real device

[Miroslav Lichvar]

On Tue, Apr 23, 2019 at 05:15:43PM +0800, Hangbin Liu wrote:
> On Tue, Apr 23, 2019 at 10:31:41AM +0200, Miroslav Lichvar wrote:
> > V2_L4_SYNC is already selected, only the following filters could be
> > selected on the macvlan interface:
> > 
> > 	HWTSTAMP_FILTER_PTP_V2_L4_SYNC,
> > 	HWTSTAMP_FILTER_PTP_V2_L4_EVENT,
> > 	HWTSTAMP_FILTER_PTP_V2_SYNC,
> > 	HWTSTAMP_FILTER_PTP_V2_EVENT,
> > 	HWTSTAMP_FILTER_ALL,
> > 
> > I think one way to check this would be to assign each filter a
> > (16-bit?) value where the individual bits correspond to the message
> > types and the newly selected filter would have to contain all bits of
> > the old one.
> 
> Just like I said, how to compare with different types.

If those values I described above were in an array called ts_map
indexed by the RX filter enum, I think the check could just be:

	(ts_map[old_filter] & ts_map[new_filter]) == tsmap[old_filter]

The individual bits would correspond to:

PTP_V1_L4_SYNC
PTP_V1_L4_DELAY_REQ
PTP_V2_L4_SYNC
PTP_V2_L4_DELAY_REQ
PTP_V2_L2_SYNC
PTP_V2_L2_DELAY_REQ
NTP_ALL

And the remaining RX filters would be combinations of those.

-- 
Miroslav Lichvar

Re: [PATCH net-next] macvlan: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to real device

[Hangbin Liu]

On Tue, Apr 23, 2019 at 11:32:13AM +0200, Miroslav Lichvar wrote:
> If those values I described above were in an array called ts_map
> indexed by the RX filter enum, I think the check could just be:
> 
> 	(ts_map[old_filter] & ts_map[new_filter]) == tsmap[old_filter]
> 
> The individual bits would correspond to:
> 
> PTP_V1_L4_SYNC
> PTP_V1_L4_DELAY_REQ
> PTP_V2_L4_SYNC
> PTP_V2_L4_DELAY_REQ
> PTP_V2_L2_SYNC
> PTP_V2_L2_DELAY_REQ
> NTP_ALL
> 
> And the remaining RX filters would be combinations of those.
> 
> -- 
Hi Miroslav, Richard,

Here is a draft patch with your idea. I haven't test it and it may has some
issues. But the logic should looks like what you said. The copy_from/to_user
is a little ugly, but I haven't come up with a more gentle way.

Would you please help have a look at it and see which way we should use?
Drop SIOCSHWTSTAMP in container or add a filter on macvlan(maybe only in
container)?

Thanks
Hangbin

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 4a6be8fab884..0f87a42fc61c 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -824,18 +824,75 @@ static int macvlan_change_mtu(struct net_device *dev, int new_mtu)
 	return 0;
 }
 
+int check_rx_filter(unsigned int new_filter, unsigned int old_filter)
+{
+	u8 ts_map[HWTSTAMP_FILTER_NTP_ALL];
+
+	memset(ts_map, 0, sizeof(ts_map));
+
+	ts_map[HWTSTAMP_FILTER_PTP_V1_L4_SYNC - 1] = 0x01;
+	ts_map[HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ - 1] = 0x02;
+	ts_map[HWTSTAMP_FILTER_PTP_V1_L4_EVENT - 1] = 0x03;
+
+	ts_map[HWTSTAMP_FILTER_PTP_V2_L4_SYNC - 1] = 0x11;
+	ts_map[HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ - 1] = 0x12;
+	ts_map[HWTSTAMP_FILTER_PTP_V2_L4_EVENT - 1] = 0x13;
+
+	ts_map[HWTSTAMP_FILTER_PTP_V2_SYNC - 1] = 0x31;
+	ts_map[HWTSTAMP_FILTER_PTP_V2_DELAY_REQ - 1] = 0x32;
+	ts_map[HWTSTAMP_FILTER_PTP_V2_EVENT - 1] = 0x33;
+
+	ts_map[HWTSTAMP_FILTER_NTP_ALL - 1] = 0xF0;
+	ts_map[HWTSTAMP_FILTER_ALL - 1] = 0xFF;
+
+	if ((ts_map[new_filter] & ts_map[old_filter]) == ts_map[old_filter])
+		return 0;
+	else
+		return -EACCES;
+}
+
 static int macvlan_do_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 {
 	struct net_device *real_dev = macvlan_dev_real_dev(dev);
 	const struct net_device_ops *ops = real_dev->netdev_ops;
-	struct ifreq ifrr;
+	unsigned int old_filter, new_filter, new_tx_type;
+	struct hwtstamp_config new_stmpconf, old_stmpconf;
 	int err = -EOPNOTSUPP;
+	struct ifreq ifrr;
+
+	/* Get new rx_filter */
+	if (copy_from_user(&new_stmpconf, ifr->ifr_data, sizeof(new_stmpconf))) {
+		return -EFAULT;
+	} else {
+		new_tx_type = new_stmpconf.tx_type;
+		new_filter = new_stmpconf.rx_filter;
+	}
 
+	/* Get old rx_filter */
 	strncpy(ifrr.ifr_name, real_dev->name, IFNAMSIZ);
 	ifrr.ifr_ifru = ifr->ifr_ifru;
 
+	if (netif_device_present(real_dev) && ops->ndo_do_ioctl)
+		err = ops->ndo_do_ioctl(real_dev, &ifrr, SIOCGHWTSTAMP);
+
+	if (!err && copy_from_user(&old_stmpconf, ifrr.ifr_data, sizeof(old_stmpconf)))
+		old_filter = old_stmpconf.rx_filter;
+	else
+		return err;
+
+	/* Copy new data back */
+	if (copy_to_user(ifrr.ifr_data, &new_stmpconf, sizeof(new_stmpconf)))
+		return -EFAULT;
+
 	switch (cmd) {
 	case SIOCSHWTSTAMP:
+		if (new_tx_type != HWTSTAMP_TX_ON ||
+		    new_filter == HWTSTAMP_FILTER_SOME)
+			return err;
+
+		err = check_rx_filter(new_filter, old_filter);
+		if (err)
+			break;
 	case SIOCGHWTSTAMP:
 		if (netif_device_present(real_dev) && ops->ndo_do_ioctl)
 			err = ops->ndo_do_ioctl(real_dev, &ifrr, cmd);
-- 
2.19.2

Re: [PATCH net-next] macvlan: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to real device

[Hangbin Liu]

On Thu, Apr 25, 2019 at 09:40:06PM +0800, Hangbin Liu wrote:
> On Tue, Apr 23, 2019 at 11:32:13AM +0200, Miroslav Lichvar wrote:
> > If those values I described above were in an array called ts_map
> > indexed by the RX filter enum, I think the check could just be:
> > 
> > 	(ts_map[old_filter] & ts_map[new_filter]) == tsmap[old_filter]
> > 
> > The individual bits would correspond to:
> > 
> > PTP_V1_L4_SYNC
> > PTP_V1_L4_DELAY_REQ
> > PTP_V2_L4_SYNC
> > PTP_V2_L4_DELAY_REQ
> > PTP_V2_L2_SYNC
> > PTP_V2_L2_DELAY_REQ
> > NTP_ALL
> > 
> > And the remaining RX filters would be combinations of those.
> > 
> > -- 
> Hi Miroslav, Richard,
> 
> Here is a draft patch with your idea. I haven't test it and it may has some
> issues. But the logic should looks like what you said. The copy_from/to_user
> is a little ugly, but I haven't come up with a more gentle way.
> 
> Would you please help have a look at it and see which way we should use?
> Drop SIOCSHWTSTAMP in container or add a filter on macvlan(maybe only in
> container)?

Hi Richard,

Any suggestion?

Thanks
Hangbin

> 
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index 4a6be8fab884..0f87a42fc61c 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -824,18 +824,75 @@ static int macvlan_change_mtu(struct net_device *dev, int new_mtu)
>  	return 0;
>  }
>  
> +int check_rx_filter(unsigned int new_filter, unsigned int old_filter)
> +{
> +	u8 ts_map[HWTSTAMP_FILTER_NTP_ALL];
> +
> +	memset(ts_map, 0, sizeof(ts_map));
> +
> +	ts_map[HWTSTAMP_FILTER_PTP_V1_L4_SYNC - 1] = 0x01;
> +	ts_map[HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ - 1] = 0x02;
> +	ts_map[HWTSTAMP_FILTER_PTP_V1_L4_EVENT - 1] = 0x03;
> +
> +	ts_map[HWTSTAMP_FILTER_PTP_V2_L4_SYNC - 1] = 0x11;
> +	ts_map[HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ - 1] = 0x12;
> +	ts_map[HWTSTAMP_FILTER_PTP_V2_L4_EVENT - 1] = 0x13;
> +
> +	ts_map[HWTSTAMP_FILTER_PTP_V2_SYNC - 1] = 0x31;
> +	ts_map[HWTSTAMP_FILTER_PTP_V2_DELAY_REQ - 1] = 0x32;
> +	ts_map[HWTSTAMP_FILTER_PTP_V2_EVENT - 1] = 0x33;
> +
> +	ts_map[HWTSTAMP_FILTER_NTP_ALL - 1] = 0xF0;
> +	ts_map[HWTSTAMP_FILTER_ALL - 1] = 0xFF;
> +
> +	if ((ts_map[new_filter] & ts_map[old_filter]) == ts_map[old_filter])
> +		return 0;
> +	else
> +		return -EACCES;
> +}
> +
>  static int macvlan_do_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
>  {
>  	struct net_device *real_dev = macvlan_dev_real_dev(dev);
>  	const struct net_device_ops *ops = real_dev->netdev_ops;
> -	struct ifreq ifrr;
> +	unsigned int old_filter, new_filter, new_tx_type;
> +	struct hwtstamp_config new_stmpconf, old_stmpconf;
>  	int err = -EOPNOTSUPP;
> +	struct ifreq ifrr;
> +
> +	/* Get new rx_filter */
> +	if (copy_from_user(&new_stmpconf, ifr->ifr_data, sizeof(new_stmpconf))) {
> +		return -EFAULT;
> +	} else {
> +		new_tx_type = new_stmpconf.tx_type;
> +		new_filter = new_stmpconf.rx_filter;
> +	}
>  
> +	/* Get old rx_filter */
>  	strncpy(ifrr.ifr_name, real_dev->name, IFNAMSIZ);
>  	ifrr.ifr_ifru = ifr->ifr_ifru;
>  
> +	if (netif_device_present(real_dev) && ops->ndo_do_ioctl)
> +		err = ops->ndo_do_ioctl(real_dev, &ifrr, SIOCGHWTSTAMP);
> +
> +	if (!err && copy_from_user(&old_stmpconf, ifrr.ifr_data, sizeof(old_stmpconf)))
> +		old_filter = old_stmpconf.rx_filter;
> +	else
> +		return err;
> +
> +	/* Copy new data back */
> +	if (copy_to_user(ifrr.ifr_data, &new_stmpconf, sizeof(new_stmpconf)))
> +		return -EFAULT;
> +
>  	switch (cmd) {
>  	case SIOCSHWTSTAMP:
> +		if (new_tx_type != HWTSTAMP_TX_ON ||
> +		    new_filter == HWTSTAMP_FILTER_SOME)
> +			return err;
> +
> +		err = check_rx_filter(new_filter, old_filter);
> +		if (err)
> +			break;
>  	case SIOCGHWTSTAMP:
>  		if (netif_device_present(real_dev) && ops->ndo_do_ioctl)
>  			err = ops->ndo_do_ioctl(real_dev, &ifrr, cmd);
> -- 
> 2.19.2
> 

Re: [PATCH net-next] macvlan: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to real device

[Richard Cochran]

On Thu, Apr 25, 2019 at 09:40:06PM +0800, Hangbin Liu wrote:
> Would you please help have a look at it and see which way we should use?
> Drop SIOCSHWTSTAMP in container or add a filter on macvlan(maybe only in
> container)?

I vote for dropping SIOCSHWTSTAMP altogether.  Why?  Because the
filter idea means that the ioctl will magically succeed or fail, based
on the unknowable state of the container's host.  It is better IMHO to
let the admin of the host set up HWTSTAMP globally (like with
hwtstamp_ctl for example) and configure the apps appropriately (like
with ptp4l --hwts_filter=check).

(BTW the patch has issues, but I'll let the advocates of the filter
idea do the review ;)

Thanks,
Richard

Re: [PATCH net-next] macvlan: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to real device

[Miroslav Lichvar]

On Mon, May 06, 2019 at 07:01:23AM -0700, Richard Cochran wrote:
> On Thu, Apr 25, 2019 at 09:40:06PM +0800, Hangbin Liu wrote:
> > Would you please help have a look at it and see which way we should use?
> > Drop SIOCSHWTSTAMP in container or add a filter on macvlan(maybe only in
> > container)?
> 
> I vote for dropping SIOCSHWTSTAMP altogether.  Why?  Because the
> filter idea means that the ioctl will magically succeed or fail, based
> on the unknowable state of the container's host.

That's a good point. I agree that SIOCSHWTSTAMP always failing would
be a less surprising behavior than failing only with some specific
configurations.

> It is better IMHO to
> let the admin of the host set up HWTSTAMP globally (like with
> hwtstamp_ctl for example) and configure the apps appropriately (like
> with ptp4l --hwts_filter=check).

Makes sense to me.

Some applications that support HW timestamping cannot do that
currently (they call SIOCSHWTSTAMP even if nothing is changing), but
it shouldn't be difficult to add support for this case.

Thanks,

-- 
Miroslav Lichvar

Re: [PATCH net-next] macvlan: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to real device

[Hangbin Liu]

On Tue, May 07, 2019 at 10:35:59AM +0200, Miroslav Lichvar wrote:
> On Mon, May 06, 2019 at 07:01:23AM -0700, Richard Cochran wrote:
> > On Thu, Apr 25, 2019 at 09:40:06PM +0800, Hangbin Liu wrote:
> > > Would you please help have a look at it and see which way we should use?
> > > Drop SIOCSHWTSTAMP in container or add a filter on macvlan(maybe only in
> > > container)?
> > 
> > I vote for dropping SIOCSHWTSTAMP altogether.  Why?  Because the
> > filter idea means that the ioctl will magically succeed or fail, based
> > on the unknowable state of the container's host.
> 
> That's a good point. I agree that SIOCSHWTSTAMP always failing would
> be a less surprising behavior than failing only with some specific
> configurations.

Thanks for the reply. As net-next is closed now. I will post the fix
to net branch after merging finished.

Cheers
Hangbin

Re: [PATCH net-next] macvlan: pass get_ts_info and SIOC[SG]HWTSTAMP ioctl to real device

[Michal Kubecek]

On Wed, May 08, 2019 at 09:41:59AM +0800, Hangbin Liu wrote:
> On Tue, May 07, 2019 at 10:35:59AM +0200, Miroslav Lichvar wrote:
> > On Mon, May 06, 2019 at 07:01:23AM -0700, Richard Cochran wrote:
> > > On Thu, Apr 25, 2019 at 09:40:06PM +0800, Hangbin Liu wrote:
> > > > Would you please help have a look at it and see which way we should use?
> > > > Drop SIOCSHWTSTAMP in container or add a filter on macvlan(maybe only in
> > > > container)?
> > > 
> > > I vote for dropping SIOCSHWTSTAMP altogether.  Why?  Because the
> > > filter idea means that the ioctl will magically succeed or fail, based
> > > on the unknowable state of the container's host.
> > 
> > That's a good point. I agree that SIOCSHWTSTAMP always failing would
> > be a less surprising behavior than failing only with some specific
> > configurations.
> 
> Thanks for the reply. As net-next is closed now. I will post the fix
> to net branch after merging finished.

net-next has been already merged into master and net so if it's a fix,
you don't have to wait (and you shouldn't).

Michal Kubecek