* [PATCH 2/6] usb: gadget: f_ncm: remove spurious boolean timer_stopping
2021-07-01 11:48 [PATCH 1/6] usb: gadget: f_ncm: remove timer_force_tx field Maciej Żenczykowski
@ 2021-07-01 11:48 ` Maciej Żenczykowski
2021-07-01 11:48 ` [PATCH 3/6] usb: gadget: f_ncm: remove check for NULL skb_tx_data in timer function Maciej Żenczykowski
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Maciej Żenczykowski @ 2021-07-01 11:48 UTC (permalink / raw)
To: Maciej Żenczykowski
Cc: Linux USB Mailing List, Brooke Basile, Bryan O'Donoghue,
Felipe Balbi, Greg Kroah-Hartman, Lorenzo Colitti
From: Maciej Żenczykowski <maze@google.com>
It is equivalent to ncm->netdev being NULL.
Cc: Brooke Basile <brookebasile@gmail.com>
Cc: "Bryan O'Donoghue" <bryan.odonoghue@linaro.org>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Lorenzo Colitti <lorenzo@google.com>
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
drivers/usb/gadget/function/f_ncm.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
index afbe70bc8d6b..e45a938424a4 100644
--- a/drivers/usb/gadget/function/f_ncm.c
+++ b/drivers/usb/gadget/function/f_ncm.c
@@ -73,7 +73,6 @@ struct f_ncm {
struct sk_buff *skb_tx_ndp;
u16 ndp_dgram_count;
struct hrtimer task_timer;
- bool timer_stopping;
};
static inline struct f_ncm *func_to_ncm(struct usb_function *f)
@@ -889,7 +888,6 @@ static int ncm_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
if (ncm->port.in_ep->enabled) {
DBG(cdev, "reset ncm\n");
- ncm->timer_stopping = true;
ncm->netdev = NULL;
gether_disconnect(&ncm->port);
ncm_reset_values(ncm);
@@ -927,7 +925,6 @@ static int ncm_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
if (IS_ERR(net))
return PTR_ERR(net);
ncm->netdev = net;
- ncm->timer_stopping = false;
}
spin_lock(&ncm->lock);
@@ -1157,16 +1154,19 @@ static struct sk_buff *ncm_wrap_ntb(struct gether *port,
static enum hrtimer_restart ncm_tx_timeout(struct hrtimer *data)
{
struct f_ncm *ncm = container_of(data, struct f_ncm, task_timer);
+ struct net_device *netdev = READ_ONCE(ncm->netdev);
/* Only send if data is available. */
- if (!ncm->timer_stopping && ncm->skb_tx_data) {
+ if (netdev && ncm->skb_tx_data) {
/* XXX This allowance of a NULL skb argument to ndo_start_xmit
* XXX is not sane. The gadget layer should be redesigned so
* XXX that the dev->wrap() invocations to build SKBs is transparent
* XXX and performed in some way outside of the ndo_start_xmit
* XXX interface.
+ *
+ * This will call directly into u_ether's eth_start_xmit()
*/
- ncm->netdev->netdev_ops->ndo_start_xmit(NULL, ncm->netdev);
+ netdev->netdev_ops->ndo_start_xmit(NULL, netdev);
}
return HRTIMER_NORESTART;
}
@@ -1355,7 +1355,6 @@ static void ncm_disable(struct usb_function *f)
DBG(cdev, "ncm deactivated\n");
if (ncm->port.in_ep->enabled) {
- ncm->timer_stopping = true;
ncm->netdev = NULL;
gether_disconnect(&ncm->port);
}
--
2.32.0.93.g670b81a890-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/6] usb: gadget: f_ncm: remove check for NULL skb_tx_data in timer function
2021-07-01 11:48 [PATCH 1/6] usb: gadget: f_ncm: remove timer_force_tx field Maciej Żenczykowski
2021-07-01 11:48 ` [PATCH 2/6] usb: gadget: f_ncm: remove spurious boolean timer_stopping Maciej Żenczykowski
@ 2021-07-01 11:48 ` Maciej Żenczykowski
2021-07-01 11:48 ` [PATCH 4/6] usb: gadget: f_ncm: remove spurious if statement Maciej Żenczykowski
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Maciej Żenczykowski @ 2021-07-01 11:48 UTC (permalink / raw)
To: Maciej Żenczykowski
Cc: Linux USB Mailing List, Brooke Basile, Bryan O'Donoghue,
Felipe Balbi, Greg Kroah-Hartman, Lorenzo Colitti
From: Maciej Żenczykowski <maze@google.com>
This condition is already checked for in ncm_wrap_ntb(),
except that that check is done with eth_dev->lock held
(it is grabbed by eth_start_xmit).
It's best to not be reaching into ncm struct without locks held.
Cc: Brooke Basile <brookebasile@gmail.com>
Cc: "Bryan O'Donoghue" <bryan.odonoghue@linaro.org>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Lorenzo Colitti <lorenzo@google.com>
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
drivers/usb/gadget/function/f_ncm.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
index e45a938424a4..77f55b3c805a 100644
--- a/drivers/usb/gadget/function/f_ncm.c
+++ b/drivers/usb/gadget/function/f_ncm.c
@@ -1156,8 +1156,7 @@ static enum hrtimer_restart ncm_tx_timeout(struct hrtimer *data)
struct f_ncm *ncm = container_of(data, struct f_ncm, task_timer);
struct net_device *netdev = READ_ONCE(ncm->netdev);
- /* Only send if data is available. */
- if (netdev && ncm->skb_tx_data) {
+ if (netdev) {
/* XXX This allowance of a NULL skb argument to ndo_start_xmit
* XXX is not sane. The gadget layer should be redesigned so
* XXX that the dev->wrap() invocations to build SKBs is transparent
--
2.32.0.93.g670b81a890-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 4/6] usb: gadget: f_ncm: remove spurious if statement
2021-07-01 11:48 [PATCH 1/6] usb: gadget: f_ncm: remove timer_force_tx field Maciej Żenczykowski
2021-07-01 11:48 ` [PATCH 2/6] usb: gadget: f_ncm: remove spurious boolean timer_stopping Maciej Żenczykowski
2021-07-01 11:48 ` [PATCH 3/6] usb: gadget: f_ncm: remove check for NULL skb_tx_data in timer function Maciej Żenczykowski
@ 2021-07-01 11:48 ` Maciej Żenczykowski
2021-07-01 11:48 ` [PATCH 5/6] usb: gadget: f_ncm: ncm_wrap_ntb - move var definitions into " Maciej Żenczykowski
2021-07-01 11:48 ` [PATCH 6/6] usb: gadget: u_ether: fix a potential null pointer dereference Maciej Żenczykowski
4 siblings, 0 replies; 6+ messages in thread
From: Maciej Żenczykowski @ 2021-07-01 11:48 UTC (permalink / raw)
To: Maciej Żenczykowski
Cc: Linux USB Mailing List, Brooke Basile, Bryan O'Donoghue,
Felipe Balbi, Greg Kroah-Hartman, Lorenzo Colitti
From: Maciej Żenczykowski <maze@google.com>
the current logic is:
struct sk_buff *skb2 = NULL;
...
if (!skb && !ncm->skb_tx_data)
return NULL;
if (skb) {
...
} else if (ncm->skb_tx_data)
...
}
return skb2;
Which means that first if statement is simply not needed.
Cc: Brooke Basile <brookebasile@gmail.com>
Cc: "Bryan O'Donoghue" <bryan.odonoghue@linaro.org>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Lorenzo Colitti <lorenzo@google.com>
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
drivers/usb/gadget/function/f_ncm.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
index 77f55b3c805a..cab17ae4fa34 100644
--- a/drivers/usb/gadget/function/f_ncm.c
+++ b/drivers/usb/gadget/function/f_ncm.c
@@ -1025,9 +1025,6 @@ static struct sk_buff *ncm_wrap_ntb(struct gether *port,
const int rem = le16_to_cpu(ntb_parameters.wNdpInPayloadRemainder);
const int dgram_idx_len = 2 * 2 * opts->dgram_item_len;
- if (!skb && !ncm->skb_tx_data)
- return NULL;
-
if (skb) {
/* Add the CRC if required up front */
if (ncm->is_crc) {
--
2.32.0.93.g670b81a890-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 5/6] usb: gadget: f_ncm: ncm_wrap_ntb - move var definitions into if statement
2021-07-01 11:48 [PATCH 1/6] usb: gadget: f_ncm: remove timer_force_tx field Maciej Żenczykowski
` (2 preceding siblings ...)
2021-07-01 11:48 ` [PATCH 4/6] usb: gadget: f_ncm: remove spurious if statement Maciej Żenczykowski
@ 2021-07-01 11:48 ` Maciej Żenczykowski
2021-07-01 11:48 ` [PATCH 6/6] usb: gadget: u_ether: fix a potential null pointer dereference Maciej Żenczykowski
4 siblings, 0 replies; 6+ messages in thread
From: Maciej Żenczykowski @ 2021-07-01 11:48 UTC (permalink / raw)
To: Maciej Żenczykowski
Cc: Linux USB Mailing List, Brooke Basile, Bryan O'Donoghue,
Felipe Balbi, Greg Kroah-Hartman, Lorenzo Colitti
From: Maciej Żenczykowski <maze@google.com>
Since they're only used if there's an skb.
Cc: Brooke Basile <brookebasile@gmail.com>
Cc: "Bryan O'Donoghue" <bryan.odonoghue@linaro.org>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Lorenzo Colitti <lorenzo@google.com>
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
drivers/usb/gadget/function/f_ncm.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
index cab17ae4fa34..dc8f078f918c 100644
--- a/drivers/usb/gadget/function/f_ncm.c
+++ b/drivers/usb/gadget/function/f_ncm.c
@@ -1013,19 +1013,20 @@ static struct sk_buff *ncm_wrap_ntb(struct gether *port,
{
struct f_ncm *ncm = func_to_ncm(&port->func);
struct sk_buff *skb2 = NULL;
- int ncb_len = 0;
- __le16 *ntb_data;
- __le16 *ntb_ndp;
- int dgram_pad;
-
- unsigned max_size = ncm->port.fixed_in_len;
- const struct ndp_parser_opts *opts = ncm->parser_opts;
- const int ndp_align = le16_to_cpu(ntb_parameters.wNdpInAlignment);
- const int div = le16_to_cpu(ntb_parameters.wNdpInDivisor);
- const int rem = le16_to_cpu(ntb_parameters.wNdpInPayloadRemainder);
- const int dgram_idx_len = 2 * 2 * opts->dgram_item_len;
if (skb) {
+ int ncb_len = 0;
+ __le16 *ntb_data;
+ __le16 *ntb_ndp;
+ int dgram_pad;
+
+ unsigned max_size = ncm->port.fixed_in_len;
+ const struct ndp_parser_opts *opts = ncm->parser_opts;
+ const int ndp_align = le16_to_cpu(ntb_parameters.wNdpInAlignment);
+ const int div = le16_to_cpu(ntb_parameters.wNdpInDivisor);
+ const int rem = le16_to_cpu(ntb_parameters.wNdpInPayloadRemainder);
+ const int dgram_idx_len = 2 * 2 * opts->dgram_item_len;
+
/* Add the CRC if required up front */
if (ncm->is_crc) {
uint32_t crc;
--
2.32.0.93.g670b81a890-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 6/6] usb: gadget: u_ether: fix a potential null pointer dereference
2021-07-01 11:48 [PATCH 1/6] usb: gadget: f_ncm: remove timer_force_tx field Maciej Żenczykowski
` (3 preceding siblings ...)
2021-07-01 11:48 ` [PATCH 5/6] usb: gadget: f_ncm: ncm_wrap_ntb - move var definitions into " Maciej Żenczykowski
@ 2021-07-01 11:48 ` Maciej Żenczykowski
4 siblings, 0 replies; 6+ messages in thread
From: Maciej Żenczykowski @ 2021-07-01 11:48 UTC (permalink / raw)
To: Maciej Żenczykowski
Cc: Linux USB Mailing List, Brooke Basile, Bryan O'Donoghue,
Felipe Balbi, Greg Kroah-Hartman, Lorenzo Colitti
From: Maciej Żenczykowski <maze@google.com>
f_ncm tx timeout can call us with null skb to flush
a pending frame. In this case skb is NULL to begin
with but ceases to be null after dev->wrap() completes.
In such a case in->maxpacket will be read, even though
we've failed to check that 'in' is not NULL.
Though I've never observed this fail in practice,
however the 'flush operation' simply does not make sense with
a null usb IN endpoint - there's nowhere to flush to...
(note that we're the gadget/device, and IN is from the point
of view of the host, so here IN actually means outbound...)
Cc: Brooke Basile <brookebasile@gmail.com>
Cc: "Bryan O'Donoghue" <bryan.odonoghue@linaro.org>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Lorenzo Colitti <lorenzo@google.com>
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
drivers/usb/gadget/function/u_ether.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
index d1d044d9f859..85a3f6d4b5af 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -492,8 +492,9 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
}
spin_unlock_irqrestore(&dev->lock, flags);
- if (skb && !in) {
- dev_kfree_skb_any(skb);
+ if (!in) {
+ if (skb)
+ dev_kfree_skb_any(skb);
return NETDEV_TX_OK;
}
--
2.32.0.93.g670b81a890-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread