[PATCH RFC] mac80211: Use IFF_ECHO to force delivery of tx_status frames

[PATCH RFC] mac80211: Use IFF_ECHO to force delivery of tx_status frames

[Julius Niedworok]

At Technical University of Munich we use MAC 802.11 TX status frames to
perform several measurements in MAC 802.11 setups.

With ath based drivers this was possible until commit d94a461d7a7df6
("ath9k: use ieee80211_tx_status_noskb where possible") as the driver
ignored the IEEE80211_TX_CTL_REQ_TX_STATUS flag and always delivered
TX status frames. Since this commit, this behavior was changed and the
driver now adheres to IEEE80211_TX_CTL_REQ_TX_STATUS.

Due to performance reasons, IEEE80211_TX_CTL_REQ_TX_STATUS is not set for
data frames from interfaces in managed mode. Hence, frames that are sent
from a managed mode interface do never deliver TX status frames. This
remains true even if a monitor mode interface (e.g. a measurement
interface) is added to the same wireless hardware device. Thus, there is
no possibility for receiving TX status frames for frames sent on an
interface in managed mode if the driver adheres to
IEEE80211_TX_CTL_REQ_TX_STATUS.

In order to force delivery of TX status frames for research and debugging
purposes, implement the IFF_ECHO flag for ieee80211 devices. When this flag
is set for a specific interface, IEEE80211_TX_CTL_REQ_TX_STATUS is enabled
in all packets sent from that interface. IFF_ECHO can be set via
/sys/class/net/<dev>/flags. The default is disabled.

Co-developed-by: Charlie Groh <ga58taw@mytum.de>
Signed-off-by: Charlie Groh <ga58taw@mytum.de>
Signed-off-by: Julius Niedworok <julius.n@gmx.net>
---
 net/core/dev.c    | 8 ++++++++
 net/mac80211/tx.c | 6 ++++++
 2 files changed, 14 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 8e276e0..076b556 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7543,6 +7543,14 @@ int __dev_change_flags(struct net_device *dev, unsigned int flags,
 			       IFF_AUTOMEDIA)) |
 		     (dev->flags & (IFF_UP | IFF_VOLATILE | IFF_PROMISC |
 				    IFF_ALLMULTI));
+	/*
+	 *	Force TX status frames on ieee80211 devices.
+	 *	Since IFF_ECHO is used by CAN devices for a different
+	 *	purpose, we must check dev->ieee80211_ptr.
+	 */
+
+	if (dev->ieee80211_ptr)
+		dev->flags = (dev->flags & ~IFF_ECHO) | (flags & IFF_ECHO);
 
 	/*
 	 *	Load in the correct multicast list now the flags have changed.
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 928f13a..2a02f66 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2463,6 +2463,9 @@ static struct sk_buff *ieee80211_build_hdr(struct ieee80211_sub_if_data *sdata,
 	if (IS_ERR(sta))
 		sta = NULL;
 
+	if (sdata->dev->flags & IFF_ECHO)
+		info_flags |= IEEE80211_TX_CTL_REQ_TX_STATUS;
+
 	/* convert Ethernet header to proper 802.11 header (based on
 	 * operation mode) */
 	ethertype = (skb->data[12] << 8) | skb->data[13];
@@ -3468,6 +3471,9 @@ static bool ieee80211_xmit_fast(struct ieee80211_sub_if_data *sdata,
 		      (tid_tx ? IEEE80211_TX_CTL_AMPDU : 0);
 	info->control.flags = IEEE80211_TX_CTRL_FAST_XMIT;
 
+	if (sdata->dev->flags & IFF_ECHO)
+		info->flags |= IEEE80211_TX_CTL_REQ_TX_STATUS;
+
 	if (hdr->frame_control & cpu_to_le16(IEEE80211_STYPE_QOS_DATA)) {
 		tid = skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
 		*ieee80211_get_qos_ctl(hdr) = tid;
-- 
2.10.1 (Apple Git-78)

Re: [PATCH RFC] mac80211: Use IFF_ECHO to force delivery of tx_status frames

[Oliver Hartkopp]

Hi Julius,

On 26.02.19 10:40, Julius Niedworok wrote:

> In order to force delivery of TX status frames for research and debugging
> purposes, implement the IFF_ECHO flag for ieee80211 devices. When this flag
> is set for a specific interface, IEEE80211_TX_CTL_REQ_TX_STATUS is enabled
> in all packets sent from that interface. IFF_ECHO can be set via
> /sys/class/net/<dev>/flags. The default is disabled.

(..)

> +	/*
> +	 *	Force TX status frames on ieee80211 devices.
> +	 *	Since IFF_ECHO is used by CAN devices for a different
> +	 *	purpose, we must check dev->ieee80211_ptr.
> +	 */

The reason for IFF_ECHO was, that the data frame which is sent onto the 
wire (by one application) is not visible to all the other applications 
on the same (local) host. Therefore a successful transmission on the 
wire triggers the 'echo' of the sent content into the local host.

So what are you getting back after you enabled IFF_ECHO on your mac80211 
device?

Is it just a 'status' about a sent packet, or is it the packet ('full 
content') itself?

Regards,
Oliver

Re: [PATCH RFC] mac80211: Use IFF_ECHO to force delivery of tx_status frames

[Julius Niedworok]

Hi Oliver,

> On 26.02.2019 12:04, Oliver Hartkopp wrote:
> 
> Hi Julius,
> 
(..)
> 
> The reason for IFF_ECHO was, that the data frame which is sent onto the wire (by one application) is not visible to all the other applications on the same (local) host. Therefore a successful transmission on the wire triggers the 'echo' of the sent content into the local host.
> 

Thank you for the explanation - I can adjust the comment, if you like to.

> So what are you getting back after you enabled IFF_ECHO on your mac80211 device?
> 
> Is it just a 'status' about a sent packet, or is it the packet ('full content') itself?

We are actually getting back the full content of the packet. So it matches the behaviour of the 'echo' in CAN.

> 
> Regards,
> Oliver
> 


Many thanks,
Julius

Re: [PATCH RFC] mac80211: Use IFF_ECHO to force delivery of tx_status frames

[Johannes Berg]

On Tue, 2019-02-26 at 14:13 +0100, Julius Niedworok wrote:
> 
> Thank you for the explanation - I can adjust the comment, if you like to.
> 
> > So what are you getting back after you enabled IFF_ECHO on your mac80211 device?
> > 
> > Is it just a 'status' about a sent packet, or is it the packet ('full content') itself?
> 
> We are actually getting back the full content of the packet. So it
> matches the behaviour of the 'echo' in CAN.

I don't think it does, really.

In CAN, if I understand correctly, this is used for regular operation
interfaces, where you might want to run 'tcpdump', on wifi the
equivalent would be 'tcpdump -i wlan0'. This *already* implements full
visibility of outgoing and incoming frames.

Not sure how CAN even manages *not to*, but I don't really need to care
:-)

You're proposing to add this to the *monitor* interfaces and you really
should have made the flag conditional on that to make that clear.

However, even on monitor interfaces, you typically *already* see the
frames you transmitted there (as raw frames, which is the only thing you
can do).

What you're proposing is to use IFF_ECHO to show frames transmitted
through *other* interfaces on the monitor interface.

I don't think the IFF_ECHO semantics really match this.


Additionally, drivers are sort of free to ignore the REQ_TX_STATUS, or
we could in the future add ways of using the _noskb to feed back TX
status to the state machines where needed, so I'm not really sure I even
_want_ this to be set in stone in such an API.

Now, I can also see how this can be useful for debugging, but it feels
to me like this should be a driver (debug) option?

johannes

Re: [PATCH RFC] mac80211: Use IFF_ECHO to force delivery of tx_status frames

[Julius Niedworok]

> On 26.02.2019 14:33 Johannes Berg wrote
> 
> You're proposing to add this to the *monitor* interfaces and you really
> should have made the flag conditional on that to make that clear.
> 
> However, even on monitor interfaces, you typically *already* see the
> frames you transmitted there (as raw frames, which is the only thing you
> can do).
> 
Thanks for your prompt reply and thoughts on our patch.

Let us briefly describe our test setup to ensure everyone on this mailing
list is one the same page.

Our general setup looks like this:
1 $ iw wlp1s0 info
Interface wlp1s0
      ifindex 5
      wdev 0x1
      addr 4c:5e:0c:11:43:ac
      type managed
      wiphy 0
      txpower 30.00 dBm
1 $ iw phy phy0 interface add mon0 type monitor
1 $ iw phy phy0 interface add mon1 type monitor

When we send (raw) packets on mon0 using packetspammer [1] and listen on
the _other_ monitor mode interface mon1, we receive frames that were sent
on the first one:
1 $ packetspammer mon0
2 $ tcpdump -i mon1 'wlan addr2 13:22:33:44:55:66'

This is due to the fact that frames sent on mon0 are echoed back as TX
status frames, because REQ_TX_STATUS is always set for frames sent from
monitor mode interfaces.

But when we replace mon0 with an interface in managed mode (wlp1s0), the
receipt of frames stops, because in managed mode REQ_TX_STATUS is cleared
in most frames:
1 $ ifup wlp1s0
1 $ ping -I wlp1s0 192.168.254.1 # this address is not assigned to any host
2 $ tcpdump -i mon1 ‚wlan addr2 4c:5e:0c:11:43:ac‘

> What you're proposing is to use IFF_ECHO to show frames transmitted
> through *other* interfaces on the monitor interface.
> 
> I don’t think the IFF_ECHO semantics really match this.
> 
What we propose is to use IFF_ECHO to force REQ_TX_STATUS being set for all
frames sent on the interface. But you are right: The goal is that frames
transmitted through the other interface show up on the monitor interface
(but only after passing the driver). However, this is exactly how we
understand the semantics of IFF_ECHO in the kernel documentation.
> 
> Additionally, drivers are sort of free to ignore the REQ_TX_STATUS, or
> we could in the future add ways of using the _noskb to feed back TX
> status to the state machines where needed, so I'm not really sure I even
> _want_ this to be set in stone in such an API.
> 
As far as we know, drivers must return a TX status frame, if REQ_TX_STATUS
is set, but can do whatever they want, if it is clear. This is no problem for our
functionality, because we force the delivery of TX status frames by
permanently setting REQ_TX_STATUS. As long as the semantics of
REQ_TX_STATUS remains like it is now, the functionality will always be
as expected from our API.

> Now, I can also see how this can be useful for debugging, but it feels
> to me like this should be a driver (debug) option?
> 
We could also achieve the functionality by modifying the drivers but this
would mean that we had to add this functionality to every driver.
Moreover, the feature of TX status frames, how it is implemented currently
for monitor mode interfaces, is part of the mac80211 implementation. The
decision to force TX status frames for monitor mode interfaces is made in
the common mac80211 implementation.

> johannes
> 
Once again, thanks for your comments. We appreciate your response.

Julius and Charlie

[1] https://warmcat.com/git/packetspammer

Re: [PATCH RFC] mac80211: Use IFF_ECHO to force delivery of tx_status frames

[Johannes Berg]

> Let us briefly describe our test setup to ensure everyone on this mailing
> list is one the same page.
> 
> Our general setup looks like this:
> 1 $ iw wlp1s0 info
> Interface wlp1s0
>       ifindex 5
>       wdev 0x1
>       addr 4c:5e:0c:11:43:ac
>       type managed
>       wiphy 0
>       txpower 30.00 dBm
> 1 $ iw phy phy0 interface add mon0 type monitor
> 1 $ iw phy phy0 interface add mon1 type monitor
> 
> When we send (raw) packets on mon0 using packetspammer [1] and listen on
> the _other_ monitor mode interface mon1, we receive frames that were sent
> on the first one:
> 1 $ packetspammer mon0
> 2 $ tcpdump -i mon1 'wlan addr2 13:22:33:44:55:66'
> 
> This is due to the fact that frames sent on mon0 are echoed back as TX
> status frames, because REQ_TX_STATUS is always set for frames sent from
> monitor mode interfaces.

Yes, I understand :-)

> But when we replace mon0 with an interface in managed mode (wlp1s0), the
> receipt of frames stops, because in managed mode REQ_TX_STATUS is cleared
> in most frames:
> 1 $ ifup wlp1s0
> 1 $ ping -I wlp1s0 192.168.254.1 # this address is not assigned to any host
> 2 $ tcpdump -i mon1 ‚wlan addr2 4c:5e:0c:11:43:ac‘

Yes, also understand.

> > What you're proposing is to use IFF_ECHO to show frames transmitted
> > through *other* interfaces on the monitor interface.
> > 
> > I don’t think the IFF_ECHO semantics really match this.
> 
> What we propose is to use IFF_ECHO to force REQ_TX_STATUS being set for all
> frames sent on the interface. But you are right: The goal is that frames
> transmitted through the other interface show up on the monitor interface
> (but only after passing the driver). However, this is exactly how we
> understand the semantics of IFF_ECHO in the kernel documentation.

I disagree.

First of all, IFF_ECHO is only documented/used *inside* the kernel, and
cannot be set by userspace today. It's documented by CAN as such:

Documentation/networking/can.rst:

   Local Loopback of Sent Frames
   -----------------------------

   As described in :ref:`socketcan-local-loopback1` the CAN network
   device driver should
   support a local loopback functionality similar to the local echo
   e.g. of tty devices. In this case the driver flag IFF_ECHO has to be
   set to prevent the PF_CAN core from locally echoing sent frames
   (aka loopback) as fallback solution::

       dev->flags = (IFF_NOARP | IFF_ECHO);

   Note that everything here is specific to a single interface.

   Also note that it's a signal from the *driver* to the *stack* to not do
   the loopback itself, because the driver will do it.

   I think in the case of all other sockets/interfaces, the stack will do
   the echo anyway, for tcpdump etc. purposes.

   The documentation in the uapi just states:
     @IFF_ECHO: echo sent packets. Volatile.
   and makes no representation about which interface, but I'd argue that
   all the flags are specific to a single interface and thus you'd expect
   this to also be.


   Thus, I don't think this was ever intended for any cross-interface
   behaviour, even if it may be on the same physical NIC.


   > As far as we know, drivers must return a TX status frame, if REQ_TX_STATUS
> is set, but can do whatever they want, if it is clear.

Not all drivers can and do this, I believe. Some things don't work very
well if they don't do it, but I _think_ you've just been lucky and used
hardware that does in fact support it.

Also note that for some hardware that does support this, there's
sometimes significant overhead - not just the performance overhead of
actually reporting the frames, but sometimes also overhead in how the
hardware is programmed and used, and how TX status is extracted.

> This is no problem for our
> functionality, because we force the delivery of TX status frames by
> permanently setting REQ_TX_STATUS. As long as the semantics of
> REQ_TX_STATUS remains like it is now, the functionality will always be
> as expected from our API.

Sure, for now, for your specific case of ath9k :-)

> We could also achieve the functionality by modifying the drivers but this
> would mean that we had to add this functionality to every driver.
> Moreover, the feature of TX status frames, how it is implemented currently
> for monitor mode interfaces, is part of the mac80211 implementation. The
> decision to force TX status frames for monitor mode interfaces is made in
> the common mac80211 implementation.

I suppose it could be in mac80211 (perhaps debugfs?) too. I just really
don't think IFF_ECHO is the right approach.

johannes

Re: [PATCH RFC] mac80211: Use IFF_ECHO to force delivery of tx_status frames

[Julius Niedworok]

> On 01.03.2019 09:32, Johannes Berg wrote:
> 
> Thus, I don't think this was ever intended for any cross-interface
> behaviour, even if it may be on the same physical NIC.

Now we got your point. We are sorry for the confusion - it seems we 
understood that wrong.

> Not all drivers can and do this, I believe. Some things don't work very
> well if they don't do it, but I _think_ you've just been lucky and used
> hardware that does in fact support it.

If the drivers do not adhere to the API, this is a problem of the drivers,
right? Because, how can we rely on *any* functionality of the drivers when
we assume that they do not adhere to the documented interfaces?

> Also note that for some hardware that does support this, there's
> sometimes significant overhead - not just the performance overhead of
> actually reporting the frames, but sometimes also overhead in how the
> hardware is programmed and used, and how TX status is extracted.

The default of this option will be disabled. If it is turned on manually
for debugging, the performance impact is probably acceptable.

> I suppose it could be in mac80211 (perhaps debugfs?) too. I just really
> don’t think IFF_ECHO is the right approach.

We see your point. So you think we should use debugfs to enable/disable
our functionality? If so, we are happy to see that we find a way to force
REQ_TX_STATUS to one from there.

Thank you,
Julius and Charlie

[PATCH RFC v2] mac80211: debugfs option to force TX status frames

[Julius Niedworok]

At Technical University of Munich we use MAC 802.11 TX status frames to
perform several measurements in MAC 802.11 setups.

With ath based drivers this was possible until commit d94a461d7a7df6
("ath9k: use ieee80211_tx_status_noskb where possible") as the driver
ignored the IEEE80211_TX_CTL_REQ_TX_STATUS flag and always delivered
tx_status frames. Since that commit, this behavior was changed and the
driver now adheres to IEEE80211_TX_CTL_REQ_TX_STATUS.

Due to performance reasons, IEEE80211_TX_CTL_REQ_TX_STATUS is not set for
data frames from interfaces in managed mode. Hence, frames that are sent
from a managed mode interface do never deliver tx_status frames. This
remains true even if a monitor mode interface (the measurement interface)
is added to the same ieee80211 physical device. Thus, there is no
possibility for receiving tx_status frames for frames sent on an interface
in managed mode, if the driver adheres to IEEE80211_TX_CTL_REQ_TX_STATUS.

In order to force delivery of tx_status frames for research and debugging
purposes, implement a debugfs option force_tx_status for ieee80211 physical
devices. When this option is set for a physical device,
IEEE80211_TX_CTL_REQ_TX_STATUS is enabled in all packets sent from that
device. This option can be set via
/sys/kernel/debug/ieee80211/<dev>/force_tx_status. The default is disabled.

Co-developed-by: Charlie Groh <ga58taw@mytum.de>
Signed-off-by: Charlie Groh <ga58taw@mytum.de>
Signed-off-by: Julius Niedworok <julius.n@gmx.net>
---
 net/mac80211/debugfs.c     | 53 ++++++++++++++++++++++++++++++++++++++++++++++
 net/mac80211/ieee80211_i.h |  1 +
 net/mac80211/tx.c          | 10 +++++++++
 3 files changed, 64 insertions(+)

diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index 3fe541e..074b5d1 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -150,6 +150,58 @@ static const struct file_operations aqm_ops = {
 	.llseek = default_llseek,
 };
 
+static ssize_t force_tx_status_read(struct file *file,
+				    char __user *user_buf,
+			size_t count,
+			loff_t *ppos)
+{
+	struct ieee80211_local *local = file->private_data;
+	char buf[3];
+	int len = 0;
+
+	len = scnprintf(buf, sizeof(buf), "%d\n", (int)local->force_tx_status);
+
+	return simple_read_from_buffer(user_buf, count, ppos,
+				       buf, len);
+}
+
+static ssize_t force_tx_status_write(struct file *file,
+				     const char __user *user_buf,
+			 size_t count,
+			 loff_t *ppos)
+{
+	struct ieee80211_local *local = file->private_data;
+	char buf[3];
+	size_t len;
+
+	if (count > sizeof(buf))
+		return -EINVAL;
+
+	if (copy_from_user(buf, user_buf, count))
+		return -EFAULT;
+
+	buf[sizeof(buf) - 1] = '\0';
+	len = strlen(buf);
+	if (len > 0 && buf[len - 1] == '\n')
+		buf[len - 1] = 0;
+
+	if (buf[0] == '0' && buf[1] == '\0')
+		local->force_tx_status = 0;
+	else if (buf[0] == '1' && buf[1] == '\0')
+		local->force_tx_status = 1;
+	else
+		return -EINVAL;
+
+	return count;
+}
+
+static const struct file_operations force_tx_status_ops = {
+	.write = force_tx_status_write,
+	.read = force_tx_status_read,
+	.open = simple_open,
+	.llseek = default_llseek,
+};
+
 #ifdef CONFIG_PM
 static ssize_t reset_write(struct file *file, const char __user *user_buf,
 			   size_t count, loff_t *ppos)
@@ -379,6 +431,7 @@ void debugfs_hw_add(struct ieee80211_local *local)
 	DEBUGFS_ADD(hwflags);
 	DEBUGFS_ADD(user_power);
 	DEBUGFS_ADD(power);
+	DEBUGFS_ADD_MODE(force_tx_status, 0600);
 
 	if (local->ops->wake_tx_queue)
 		DEBUGFS_ADD_MODE(aqm, 0600);
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 7dfb4e2..3339b5d 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1367,6 +1367,7 @@ struct ieee80211_local {
 		struct dentry *rcdir;
 		struct dentry *keys;
 	} debugfs;
+	bool force_tx_status;
 #endif
 
 	/*
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 928f13a..717fa71 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2463,6 +2463,11 @@ static struct sk_buff *ieee80211_build_hdr(struct ieee80211_sub_if_data *sdata,
 	if (IS_ERR(sta))
 		sta = NULL;
 
+#ifdef CONFIG_MAC80211_DEBUGFS
+		if (local->force_tx_status)
+			info_flags |= IEEE80211_TX_CTL_REQ_TX_STATUS;
+#endif
+
 	/* convert Ethernet header to proper 802.11 header (based on
 	 * operation mode) */
 	ethertype = (skb->data[12] << 8) | skb->data[13];
@@ -3468,6 +3473,11 @@ static bool ieee80211_xmit_fast(struct ieee80211_sub_if_data *sdata,
 		      (tid_tx ? IEEE80211_TX_CTL_AMPDU : 0);
 	info->control.flags = IEEE80211_TX_CTRL_FAST_XMIT;
 
+#ifdef CONFIG_MAC80211_DEBUGFS
+		if (local->force_tx_status)
+			info->flags |= IEEE80211_TX_CTL_REQ_TX_STATUS;
+#endif
+
 	if (hdr->frame_control & cpu_to_le16(IEEE80211_STYPE_QOS_DATA)) {
 		tid = skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
 		*ieee80211_get_qos_ctl(hdr) = tid;
-- 
2.10.1 (Apple Git-78)

Re: [PATCH RFC v2] mac80211: debugfs option to force TX status frames

[Kalle Valo]

Julius Niedworok <julius.n@gmx.net> writes:

> At Technical University of Munich we use MAC 802.11 TX status frames to
> perform several measurements in MAC 802.11 setups.
>
> With ath based drivers this was possible until commit d94a461d7a7df6
> ("ath9k: use ieee80211_tx_status_noskb where possible") as the driver
> ignored the IEEE80211_TX_CTL_REQ_TX_STATUS flag and always delivered
> tx_status frames. Since that commit, this behavior was changed and the
> driver now adheres to IEEE80211_TX_CTL_REQ_TX_STATUS.
>
> Due to performance reasons, IEEE80211_TX_CTL_REQ_TX_STATUS is not set for
> data frames from interfaces in managed mode. Hence, frames that are sent
> from a managed mode interface do never deliver tx_status frames. This
> remains true even if a monitor mode interface (the measurement interface)
> is added to the same ieee80211 physical device. Thus, there is no
> possibility for receiving tx_status frames for frames sent on an interface
> in managed mode, if the driver adheres to IEEE80211_TX_CTL_REQ_TX_STATUS.
>
> In order to force delivery of tx_status frames for research and debugging
> purposes, implement a debugfs option force_tx_status for ieee80211 physical
> devices. When this option is set for a physical device,
> IEEE80211_TX_CTL_REQ_TX_STATUS is enabled in all packets sent from that
> device. This option can be set via
> /sys/kernel/debug/ieee80211/<dev>/force_tx_status. The default is disabled.
>
> Co-developed-by: Charlie Groh <ga58taw@mytum.de>
> Signed-off-by: Charlie Groh <ga58taw@mytum.de>
> Signed-off-by: Julius Niedworok <julius.n@gmx.net>

[...]

> +	len = scnprintf(buf, sizeof(buf), "%d\n", (int)local->force_tx_status);

I wonder about the cast, is it guaranteed that a bool is always of the
same size as an int?

> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -1367,6 +1367,7 @@ struct ieee80211_local {
>  		struct dentry *rcdir;
>  		struct dentry *keys;
>  	} debugfs;
> +	bool force_tx_status;

[...]

>  #endif
>  
>  	/*
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 928f13a..717fa71 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -2463,6 +2463,11 @@ static struct sk_buff *ieee80211_build_hdr(struct ieee80211_sub_if_data *sdata,
>  	if (IS_ERR(sta))
>  		sta = NULL;
>  
> +#ifdef CONFIG_MAC80211_DEBUGFS
> +		if (local->force_tx_status)
> +			info_flags |= IEEE80211_TX_CTL_REQ_TX_STATUS;
> +#endif
> +
>  	/* convert Ethernet header to proper 802.11 header (based on
>  	 * operation mode) */
>  	ethertype = (skb->data[12] << 8) | skb->data[13];
> @@ -3468,6 +3473,11 @@ static bool ieee80211_xmit_fast(struct ieee80211_sub_if_data *sdata,
>  		      (tid_tx ? IEEE80211_TX_CTL_AMPDU : 0);
>  	info->control.flags = IEEE80211_TX_CTRL_FAST_XMIT;
>  
> +#ifdef CONFIG_MAC80211_DEBUGFS
> +		if (local->force_tx_status)
> +			info->flags |= IEEE80211_TX_CTL_REQ_TX_STATUS;
> +#endif

IMHO the ifdefs look pointless just to save four bytes. I would move
force_tx_status outside of ifdef in the struct so that the actual code
doesn't have ugly ifdefs.

-- 
Kalle Valo

Re: [PATCH RFC v2] mac80211: debugfs option to force TX status frames

[ga58taw]

On Thu, Mar 07, 2019 at 05:42:04PM +0200, Kalle Valo wrote:
> > +   len = scnprintf(buf, sizeof(buf), "%d\n", (int)local->force_tx_status);
>
> I wonder about the cast, is it guaranteed that a bool is always of the
> same size as an int?

Why is this a problem? If a bool is smaller than an int, the compiler
emits code that will prepend the value of force_tx_status with zeros. If
it is larger than an int the compiler emits code that will truncate
force_tx_status to sizeof(int) bytes. So it doesn't matter how large bool
and/or int are, the code always prints '0' or '1' depending on the value
of force_tx_status.

> > --- a/net/mac80211/tx.c
> > +++ b/net/mac80211/tx.c
> > @@ -2463,6 +2463,11 @@ static struct sk_buff *ieee80211_build_hdr(struct ieee80211_sub_if_data *sdata,
> >     if (IS_ERR(sta))
> >             sta = NULL;
> >
> > +#ifdef CONFIG_MAC80211_DEBUGFS
> > +           if (local->force_tx_status)
> > +                   info_flags |= IEEE80211_TX_CTL_REQ_TX_STATUS;
> > +#endif
> > +
> >     /* convert Ethernet header to proper 802.11 header (based on
> >      * operation mode) */
> >     ethertype = (skb->data[12] << 8) | skb->data[13];
> > @@ -3468,6 +3473,11 @@ static bool ieee80211_xmit_fast(struct ieee80211_sub_if_data *sdata,
> >                   (tid_tx ? IEEE80211_TX_CTL_AMPDU : 0);
> >     info->control.flags = IEEE80211_TX_CTRL_FAST_XMIT;
> >
> > +#ifdef CONFIG_MAC80211_DEBUGFS
> > +           if (local->force_tx_status)
> > +                   info->flags |= IEEE80211_TX_CTL_REQ_TX_STATUS;
> > +#endif
>
> IMHO the ifdefs look pointless just to save four bytes. I would move
> force_tx_status outside of ifdef in the struct so that the actual code
> doesn't have ugly ifdefs.

I don't think ifdefs are ugly in this case. They clearly indicate that the
code belongs to the debugfs implementation of mac80211. In addition, when
we remove the ifdefs, code that only belongs to the debugfs implementation
of mac80211 is always included in the kernel (even when the config
option is turned off!) which, I think, is much more ugly than the ifdefs.

Thanks for your comments and ideas.

Charlie

Re: [PATCH RFC v2] mac80211: debugfs option to force TX status frames

[Kalle Valo]

ga58taw@mytum.de writes:

> On Thu, Mar 07, 2019 at 05:42:04PM +0200, Kalle Valo wrote:
>> > +   len = scnprintf(buf, sizeof(buf), "%d\n", (int)local->force_tx_status);
>>
>> I wonder about the cast, is it guaranteed that a bool is always of the
>> same size as an int?
>
> Why is this a problem? If a bool is smaller than an int, the compiler
> emits code that will prepend the value of force_tx_status with zeros.

Let's say that a bool is a byte and int is four bytes. If you use "%d" I
would guess that in that case scnprintf() writes 4 bytes, meaning that 3
bytes will be overwriting either padding or some other field in the
struct.

But I'm no compiler expert so I'm not going to argue about this anymore.
I just wanted to point out that that the cast looks dangerous and I
would not do it.

-- 
Kalle Valo

Re: [PATCH RFC v2] mac80211: debugfs option to force TX status frames

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 1537 bytes --]

On 2019-03-11, at 16:03:38 +0200, Kalle Valo wrote:
> ga58taw@mytum.de writes:
> > On Thu, Mar 07, 2019 at 05:42:04PM +0200, Kalle Valo wrote:
> > > > +   len = scnprintf(buf, sizeof(buf), "%d\n",
> > > >                    (int)local->force_tx_status);
> > >
> > > I wonder about the cast, is it guaranteed that a bool is always of
> > > the same size as an int?
> >
> > Why is this a problem? If a bool is smaller than an int, the
> > compiler emits code that will prepend the value of force_tx_status
> > with zeros.
>
> Let's say that a bool is a byte and int is four bytes. If you use "%d"
> I would guess that in that case scnprintf() writes 4 bytes, meaning
> that 3 bytes will be overwriting either padding or some other field in
> the struct.

It's the value that matters, not the type.  It will only be too big for
the buffer if the result of casting local->force_tx_status to int is
less than -9 or greeater than 99.

  scnprintf(buf, size(buf), "%lld\n", (long long)local->force_tx_status)

would also be fine if the value were in range.  Note also that scnprintf
will not overrun the buffer: it will truncate the string.

> But I'm no compiler expert so I'm not going to argue about this
> anymore.  I just wanted to point out that that the cast looks
> dangerous and I would not do it.

As it happens, arguments to variadic functions are subject to the
"default argument promotions," so if local->force_tx_status is in fact a
bool (I can't find the definition), it would be promoted to int and the
cast is superfluous.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC v2] mac80211: debugfs option to force TX status frames

[Julius Niedworok]

> On 11.03.2019 15:52, Jeremy Sowden wrote:
>
> It's the value that matters, not the type.  It will only be too big for
> the buffer if the result of casting local->force_tx_status to int is
> less than -9 or greeater than 99.
>
>  scnprintf(buf, size(buf), "%lld\n", (long long)local->force_tx_status)
>
> would also be fine if the value were in range.  Note also that scnprintf
> will not overrun the buffer: it will truncate the string.

Thanks for the clarification :)

> As it happens, arguments to variadic functions are subject to the
> "default argument promotions," so if local->force_tx_status is in fact a
> bool (I can't find the definition), it would be promoted to int and the
> cast is superfluous.

Yes - the cast is superfluous. We still think it might be useful to keep it
there to make the point that the value will be casted. However, if you
prefer to omit the cast, we are happy to take it out.


Thank you,
Julius and Charlie

Re: [PATCH RFC v2] mac80211: debugfs option to force TX status frames

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 1304 bytes --]

On 2019-03-19, at 16:07:32 +0100, Julius Niedworok wrote:
> On 11.03.2019 15:52, Jeremy Sowden wrote:
> > It's the value that matters, not the type.  It will only be too big
> > for the buffer if the result of casting local->force_tx_status to
> > int is less than -9 or greeater than 99.
> >
> >  scnprintf(buf, size(buf), "%lld\n",
> >            (long long)local->force_tx_status)
> >
> > would also be fine if the value were in range.  Note also that
> > scnprintf will not overrun the buffer: it will truncate the string.
>
> Thanks for the clarification :)

You're welcome. :)

> > As it happens, arguments to variadic functions are subject to the
> > "default argument promotions," so if local->force_tx_status is in
> > fact a bool (I can't find the definition), it would be promoted to
> > int and the cast is superfluous.
>
> Yes - the cast is superfluous. We still think it might be useful to
> keep it there to make the point that the value will be casted.
> However, if you prefer to omit the cast, we are happy to take it out.

If I were maintaining this code, I'd probably take it out.  However, I
did have to check the documentation to verify that the cast wasn't
required, and I know the more obscure corners of the C standard pretty
well, so it may cause less confusion to keep it.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3] mac80211: debugfs option to force TX status frames

[Julius Niedworok]

At Technical University of Munich we use MAC 802.11 TX status frames to
perform several measurements in MAC 802.11 setups.

With ath based drivers this was possible until commit d94a461d7a7df6
("ath9k: use ieee80211_tx_status_noskb where possible") as the driver
ignored the IEEE80211_TX_CTL_REQ_TX_STATUS flag and always delivered
tx_status frames. Since that commit, this behavior was changed and the
driver now adheres to IEEE80211_TX_CTL_REQ_TX_STATUS.

Due to performance reasons, IEEE80211_TX_CTL_REQ_TX_STATUS is not set for
data frames from interfaces in managed mode. Hence, frames that are sent
from a managed mode interface do never deliver tx_status frames. This
remains true even if a monitor mode interface (the measurement interface)
is added to the same ieee80211 physical device. Thus, there is no
possibility for receiving tx_status frames for frames sent on an interface
in managed mode, if the driver adheres to IEEE80211_TX_CTL_REQ_TX_STATUS.

In order to force delivery of tx_status frames for research and debugging
purposes, implement a debugfs option force_tx_status for ieee80211 physical
devices. When this option is set for a physical device,
IEEE80211_TX_CTL_REQ_TX_STATUS is enabled in all packets sent from that
device. This option can be set via
/sys/kernel/debug/ieee80211/<dev>/force_tx_status. The default is disabled.

Co-developed-by: Charlie Groh <ga58taw@mytum.de>
Signed-off-by: Charlie Groh <ga58taw@mytum.de>
Signed-off-by: Julius Niedworok <julius.n@gmx.net>
---
 net/mac80211/debugfs.c     | 53 ++++++++++++++++++++++++++++++++++++++++++++++
 net/mac80211/ieee80211_i.h |  1 +
 net/mac80211/tx.c          | 10 +++++++++
 3 files changed, 64 insertions(+)

diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index 2d43bc1..da8f27c 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -150,6 +150,58 @@ static const struct file_operations aqm_ops = {
 	.llseek = default_llseek,
 };

+static ssize_t force_tx_status_read(struct file *file,
+				    char __user *user_buf,
+				    size_t count,
+				    loff_t *ppos)
+{
+	struct ieee80211_local *local = file->private_data;
+	char buf[3];
+	int len = 0;
+
+	len = scnprintf(buf, sizeof(buf), "%d\n", (int)local->force_tx_status);
+
+	return simple_read_from_buffer(user_buf, count, ppos,
+				       buf, len);
+}
+
+static ssize_t force_tx_status_write(struct file *file,
+				     const char __user *user_buf,
+				     size_t count,
+				     loff_t *ppos)
+{
+	struct ieee80211_local *local = file->private_data;
+	char buf[3];
+	size_t len;
+
+	if (count > sizeof(buf))
+		return -EINVAL;
+
+	if (copy_from_user(buf, user_buf, count))
+		return -EFAULT;
+
+	buf[sizeof(buf) - 1] = '\0';
+	len = strlen(buf);
+	if (len > 0 && buf[len - 1] == '\n')
+		buf[len - 1] = 0;
+
+	if (buf[0] == '0' && buf[1] == '\0')
+		local->force_tx_status = 0;
+	else if (buf[0] == '1' && buf[1] == '\0')
+		local->force_tx_status = 1;
+	else
+		return -EINVAL;
+
+	return count;
+}
+
+static const struct file_operations force_tx_status_ops = {
+	.write = force_tx_status_write,
+	.read = force_tx_status_read,
+	.open = simple_open,
+	.llseek = default_llseek,
+};
+
 #ifdef CONFIG_PM
 static ssize_t reset_write(struct file *file, const char __user *user_buf,
 			   size_t count, loff_t *ppos)
@@ -382,6 +434,7 @@ void debugfs_hw_add(struct ieee80211_local *local)
 	DEBUGFS_ADD(hwflags);
 	DEBUGFS_ADD(user_power);
 	DEBUGFS_ADD(power);
+	DEBUGFS_ADD_MODE(force_tx_status, 0600);

 	if (local->ops->wake_tx_queue)
 		DEBUGFS_ADD_MODE(aqm, 0600);
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index e170f98..28ac87c 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1384,6 +1384,7 @@ struct ieee80211_local {
 		struct dentry *rcdir;
 		struct dentry *keys;
 	} debugfs;
+	bool force_tx_status;
 #endif

 	/*
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 8a49a74..a39ce49 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2470,6 +2470,11 @@ static struct sk_buff *ieee80211_build_hdr(struct ieee80211_sub_if_data *sdata,
 	if (IS_ERR(sta))
 		sta = NULL;

+#ifdef CONFIG_MAC80211_DEBUGFS
+	if (local->force_tx_status)
+		info_flags |= IEEE80211_TX_CTL_REQ_TX_STATUS;
+#endif
+
 	/* convert Ethernet header to proper 802.11 header (based on
 	 * operation mode) */
 	ethertype = (skb->data[12] << 8) | skb->data[13];
@@ -3475,6 +3480,11 @@ static bool ieee80211_xmit_fast(struct ieee80211_sub_if_data *sdata,
 		      (tid_tx ? IEEE80211_TX_CTL_AMPDU : 0);
 	info->control.flags = IEEE80211_TX_CTRL_FAST_XMIT;

+#ifdef CONFIG_MAC80211_DEBUGFS
+	if (local->force_tx_status)
+		info->flags |= IEEE80211_TX_CTL_REQ_TX_STATUS;
+#endif
+
 	if (hdr->frame_control & cpu_to_le16(IEEE80211_STYPE_QOS_DATA)) {
 		tid = skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
 		*ieee80211_get_qos_ctl(hdr) = tid;
--
2.10.1 (Apple Git-78)