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

[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] 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//flags. The default is disabled.

Co-developed-by: Charlie Groh 
Signed-off-by: Charlie Groh 
Signed-off-by: Julius Niedworok 
---
 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

[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

[PATCH RFC] net-sysfs: consistent behavior between ioctl and sysfs

[Julius Niedworok]

The flags of a network interface can be read either from
/sys/class/net//flags or from the ioctl SIOCGIFFLAGS. When the ioctl
is used, dev_get_flags is called to extract the flags. However, reads on
the sysfs file return the plain content of the flags integer and do not
call dev_get_flags.

In order to fix this inconsistent behavior, replace the NETDEVICE_SHOW_RW
macro by the functions format_flags and flags_show. These functions extract
the flags by calling dev_get_flags and return them formatted for the user.

This makes the behavior between calling the SIOCGIFFLAGS ioctl and reading
/sys/class/net//flags consistent.

Co-developed-by: Charlie Groh 
Signed-off-by: Charlie Groh 
Signed-off-by: Julius Niedworok 
---
 net/core/net-sysfs.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index ff9fd2b..8ae8be5 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -345,7 +345,19 @@ static ssize_t flags_store(struct device *dev, struct 
device_attribute *attr,
 {
return netdev_store(dev, attr, buf, len, change_flags);
 }
-NETDEVICE_SHOW_RW(flags, fmt_hex);
+
+static ssize_t format_flags(const struct net_device *dev, char *buf)
+{
+   return sprintf(buf, fmt_hex, dev_get_flags(dev));
+}
+
+static ssize_t flags_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+   return netdev_show(dev, attr, buf, format_flags);
+}
+static DEVICE_ATTR_RW(flags);
 
 static ssize_t tx_queue_len_store(struct device *dev,
  struct device_attribute *attr,
-- 
2.10.1 (Apple Git-78)

[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//force_tx_status. The default is disabled.

Co-developed-by: Charlie Groh 
Signed-off-by: Charlie Groh 
Signed-off-by: Julius Niedworok 
---
 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] <<