RE: [PATCH] xhci: fix array index out of the bounds in function last_trb() and last_trb_on_last_seg()
Then the inc_deq() code could just be:
ring-deq_updates++;
ring-num_trbs_free++;
if (last_trb(xhci, ring, ring-deq_seg, ++ring-dequeue)) {
ring-deq_seg = ring-deq_seg-next;
ring-dequeue = ring-deq_seg-trbs;
if (ring-deq_seg == ring-first_seg) /* not sure of name */
ring-cycle_state ^= 1;
}
The cycle state is only changed when the enqueue pointer makes it past
the segment with the toggle bit. It shouldn't be modified for the
dequeue pointer.
Except for the event ring.
I did think that was a quick rewrite of the existing functionality.
Really the code for the event and command rings needs separating completely.
It would all then be clearer.
And yes, inc_deq() could do with some simplification. David, are you
interested in creating a patch to simplify this code? I haven't gotten
v2 revisions from a couple of patches you previously sent, so I wasn't
sure if you're still interested in creating xHCI patches.
I will sort out my existing patches hopefully today.
git managed to trash my source tree and I've been busy fixing some
of my own code.
I was waiting for the patches I'd posted to get processed before
adding many more. Otherwise it all gets completely out of hand.
Also I've dropped you (Sarah) from the mail address list, our work email
server is getting very strange bounces for linux.intel.com and I think
the list carefully avoids delivering mails to addresses in the mail itself.
David
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] xhci: fix array index out of the bounds in function last_trb() and last_trb_on_last_seg()
From: Wang, Lin X
David is right, this patch may lead to the last trb in an event ring
unprocessed according to the
current logic, you can reject this patch, although I think index
out-of-bounds is reasonable.
If applying this patch, then corresponding function(inc_deq()) should be
modified, maybe like the
following way:
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d26cd94..0dbaa56 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -185,7 +185,7 @@ static void inc_deq(struct xhci_hcd *xhci, struct
xhci_ring *ring)
} else {
ring-dequeue++;
}
- } while (last_trb(xhci, ring, ring-deq_seg, ring-dequeue));
+ } while (last_trb(xhci, ring, ring-deq_seg, ring-dequeue)
ring-type != TYPE_EVENT);
There are far too many conditionals in this code already.
Adding an extra one that serves no purpose is silly.
The C language explicitly allows you to take the address of the first
item beyond the end of an array and to use that in expressions
involving pointers to other array members.
David
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] xhci: fix array index out of the bounds in function last_trb() and last_trb_on_last_seg()
From: Sarah Sharp
On Mon, Dec 16, 2013 at 03:20:28PM -, David Laight wrote:
I know all these difference clearly, inc_deq() is indeed a common
function for different rings, but lasr_trb() last_trb_on_last_seg()
inside it use different condition to determine the last trb in an
event ring and an non-event ring; and sorry, i still not find why last
trb in an event ring skipped by H/W according to the current logic.
Thanks!
Read the specs and the code.
State your objections clearly.
Based on my analysis, this patch will produce correct behavior with the
event ring:
http://marc.info/?l=linux-usbm=138697807827548w=2
Am I missing something?
-/* Is this TRB a link TRB or was the last TRB the last TRB in this event ring
- * segment? I.e. would the updated event TRB pointer step off the end of the
- * event seg?
+/* Is this TRB a link TRB or was the last TRB in this event ring segment?
+ * I.e. would the updated event TRB pointer step off the end of the event
+ * seg?
*/
static int last_trb(struct xhci_hcd *xhci, struct xhci_ring *ring,
struct xhci_segment *seg, union xhci_trb *trb)
{
if (ring == xhci-event_ring)
- return trb == seg-trbs[TRBS_PER_SEGMENT];
+ return trb == seg-trbs[TRBS_PER_SEGMENT - 1];
else
return TRB_TYPE_LINK_LE32(trb-link.control);
}
It would be extremely confusing for the above function to behave
differently for event and other rings.
Actually last_trb() could just be:
return trb == seg-last_trb;
provided an appropriate 'last_trb' field was set.
If we assume that there are no empty ring segments (ie adjacent LINK TRBs),
and that it is never actually called with a pointer to a link trb.
Then the inc_deq() code could just be:
ring-deq_updates++;
ring-num_trbs_free++;
if (last_trb(xhci, ring, ring-deq_seg, ++ring-dequeue)) {
ring-deq_seg = ring-deq_seg-next;
ring-dequeue = ring-deq_seg-trbs;
if (ring-deq_seg == ring-first_seg) /* not sure of name */
ring-cycle_state ^= 1;
}
David
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] xhci: fix array index out of the bounds in function last_trb() and last_trb_on_last_seg()
On Tue, Dec 17, 2013 at 09:35:39AM -, David Laight wrote:
From: Wang, Lin X
David is right, this patch may lead to the last trb in an event ring
unprocessed according to the
current logic, you can reject this patch, although I think index
out-of-bounds is reasonable.
If applying this patch, then corresponding function(inc_deq()) should be
modified, maybe like the
following way:
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d26cd94..0dbaa56 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -185,7 +185,7 @@ static void inc_deq(struct xhci_hcd *xhci, struct
xhci_ring *ring)
} else {
ring-dequeue++;
}
- } while (last_trb(xhci, ring, ring-deq_seg, ring-dequeue));
+ } while (last_trb(xhci, ring, ring-deq_seg, ring-dequeue)
ring-type != TYPE_EVENT);
Ah, ok, I see where I misread the code now. Thanks for figuring that
out, David.
There are far too many conditionals in this code already.
Adding an extra one that serves no purpose is silly.
The C language explicitly allows you to take the address of the first
item beyond the end of an array and to use that in expressions
involving pointers to other array members.
The out-of-bounds pointer math is fine as it stands. I'll simply drop
this patch.
Sarah Sharp
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] xhci: fix array index out of the bounds in function last_trb() and last_trb_on_last_seg()
From: Of Wang, Lin X
Sent: 15 December 2013 14:21
To: Sarah Sharp (sarah.a.sh...@linux.intel.com)
Cc: linux-usb@vger.kernel.org
Subject: [PATCH] xhci: fix array index out of the bounds in function
last_trb() and
last_trb_on_last_seg()
In function last_trb() and last_trb_on_last_seg(), incorrect array index
value 'TRBS_PER_SEGMENT' is
used to determine the last element in an event ring segment, which lead to
the out-of-bounds
of index. But according to the current logic of event ring operation, this
bug brings no problems
and the driver works as desired. This patch only fix this array index
out-of-bounds issue and there is
no need no modify corresponding ring operation.
Signed-off-by: Lin Wang lin.x.w...@intel.com
---
drivers/usb/host/xhci-ring.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d26cd94..3f93b07 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -98,21 +98,21 @@ static bool last_trb_on_last_seg(struct xhci_hcd *xhci,
struct xhci_ring *ring,
struct xhci_segment *seg, union xhci_trb *trb)
{
if (ring == xhci-event_ring)
- return (trb == seg-trbs[TRBS_PER_SEGMENT])
+ return (trb == seg-trbs[TRBS_PER_SEGMENT - 1])
Taking the address of the element beyond the end of an array is valid C.
So there is nothing wrong with this code.
Apply the patch and the last event written by the hardware won't be processed.
David
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] xhci: fix array index out of the bounds in function last_trb() and last_trb_on_last_seg()
2013/12/16 David Laight david.lai...@aculab.com:
From: Of Wang, Lin X
Sent: 15 December 2013 14:21
To: Sarah Sharp (sarah.a.sh...@linux.intel.com)
Cc: linux-usb@vger.kernel.org
Subject: [PATCH] xhci: fix array index out of the bounds in function
last_trb() and
last_trb_on_last_seg()
In function last_trb() and last_trb_on_last_seg(), incorrect array index
value 'TRBS_PER_SEGMENT' is
used to determine the last element in an event ring segment, which lead to
the out-of-bounds
of index. But according to the current logic of event ring operation, this
bug brings no problems
and the driver works as desired. This patch only fix this array index
out-of-bounds issue and there is
no need no modify corresponding ring operation.
Signed-off-by: Lin Wang lin.x.w...@intel.com
---
drivers/usb/host/xhci-ring.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d26cd94..3f93b07 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -98,21 +98,21 @@ static bool last_trb_on_last_seg(struct xhci_hcd *xhci,
struct xhci_ring *ring,
struct xhci_segment *seg, union xhci_trb *trb)
{
if (ring == xhci-event_ring)
- return (trb == seg-trbs[TRBS_PER_SEGMENT])
+ return (trb == seg-trbs[TRBS_PER_SEGMENT - 1])
Taking the address of the element beyond the end of an array is valid C.
It's vaild in C doesn't mean that it‘s reasonable, besides,
'TRBS_PER_SEGMENT - 1' is widely used in current implementation to
identily the last element in an array, using 'TRBS_PER_SEGMENT' here
result in
a lack of unity in style and easy to get lost.
So there is nothing wrong with this code.
Apply the patch and the last event written by the hardware won't be processed.
The only function affected here is inc_deq(), and i did not find trb
skipped by H/W, please let me know if i missed something.
Thanks！
Lin
David
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] xhci: fix array index out of the bounds in function last_trb() and last_trb_on_last_seg()
From: Wang, Lin X
2013/12/16 David Laight david.lai...@aculab.com:
From: Of Wang, Lin X
Sent: 15 December 2013 14:21
To: Sarah Sharp (sarah.a.sh...@linux.intel.com)
Cc: linux-usb@vger.kernel.org
Subject: [PATCH] xhci: fix array index out of the bounds in function
last_trb() and
last_trb_on_last_seg()
In function last_trb() and last_trb_on_last_seg(), incorrect array index
value 'TRBS_PER_SEGMENT'
is
used to determine the last element in an event ring segment, which lead to
the out-of-bounds
of index. But according to the current logic of event ring operation, this
bug brings no problems
and the driver works as desired. This patch only fix this array index
out-of-bounds issue and there
is
no need no modify corresponding ring operation.
Signed-off-by: Lin Wang lin.x.w...@intel.com
---
drivers/usb/host/xhci-ring.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d26cd94..3f93b07 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -98,21 +98,21 @@ static bool last_trb_on_last_seg(struct xhci_hcd
*xhci, struct xhci_ring *ring,
struct xhci_segment *seg, union xhci_trb *trb)
{
if (ring == xhci-event_ring)
- return (trb == seg-trbs[TRBS_PER_SEGMENT])
+ return (trb == seg-trbs[TRBS_PER_SEGMENT - 1])
Taking the address of the element beyond the end of an array is valid C.
It's vaild in C doesn't mean that it‘s reasonable, besides,
'TRBS_PER_SEGMENT - 1' is widely used in current implementation to
identily the last element in an array, using 'TRBS_PER_SEGMENT' here
result in
a lack of unity in style and easy to get lost.
There is a big difference between the 'event' ring (written by the xhci
hardware)
and all the other rings (read by the hardware).
The last entry of the non-event rings is always a LINK trb (pointing to the
next ring segment) so must not be overwritten.
The event rings have a separate array that defines the segments and the hardware
writes real data into the last ring entry.
The operation of the event ring is different from the other rings and it was
(IMHO) a mistake to have used common functions for the two.
David
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] xhci: fix array index out of the bounds in function last_trb() and last_trb_on_last_seg()
2013/12/16 David Laight david.lai...@aculab.com:
From: Wang, Lin X
2013/12/16 David Laight david.lai...@aculab.com:
From: Of Wang, Lin X
Sent: 15 December 2013 14:21
To: Sarah Sharp (sarah.a.sh...@linux.intel.com)
Cc: linux-usb@vger.kernel.org
Subject: [PATCH] xhci: fix array index out of the bounds in function
last_trb() and
last_trb_on_last_seg()
In function last_trb() and last_trb_on_last_seg(), incorrect array index
value 'TRBS_PER_SEGMENT'
is
used to determine the last element in an event ring segment, which lead
to the out-of-bounds
of index. But according to the current logic of event ring operation,
this bug brings no problems
and the driver works as desired. This patch only fix this array index
out-of-bounds issue and there
is
no need no modify corresponding ring operation.
Signed-off-by: Lin Wang lin.x.w...@intel.com
---
drivers/usb/host/xhci-ring.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d26cd94..3f93b07 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -98,21 +98,21 @@ static bool last_trb_on_last_seg(struct xhci_hcd
*xhci, struct xhci_ring *ring,
struct xhci_segment *seg, union xhci_trb *trb)
{
if (ring == xhci-event_ring)
- return (trb == seg-trbs[TRBS_PER_SEGMENT])
+ return (trb == seg-trbs[TRBS_PER_SEGMENT - 1])
Taking the address of the element beyond the end of an array is valid C.
It's vaild in C doesn't mean that it‘s reasonable, besides,
'TRBS_PER_SEGMENT - 1' is widely used in current implementation to
identily the last element in an array, using 'TRBS_PER_SEGMENT' here
result in
a lack of unity in style and easy to get lost.
There is a big difference between the 'event' ring (written by the xhci
hardware)
and all the other rings (read by the hardware).
The last entry of the non-event rings is always a LINK trb (pointing to the
next ring segment) so must not be overwritten.
The event rings have a separate array that defines the segments and the
hardware
writes real data into the last ring entry.
Hi David,
I know all these difference clearly, inc_deq() is indeed a common
function for different rings, but lasr_trb() last_trb_on_last_seg()
inside it use different condition to determine the last trb in an
event ring and an non-event ring; and sorry, i still not find why last
trb in an event ring skipped by H/W according to the current logic.
Thanks!
Lin
The operation of the event ring is different from the other rings and it was
(IMHO) a mistake to have used common functions for the two.
David
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] xhci: fix array index out of the bounds in function last_trb() and last_trb_on_last_seg()
I know all these difference clearly, inc_deq() is indeed a common function for different rings, but lasr_trb() last_trb_on_last_seg() inside it use different condition to determine the last trb in an event ring and an non-event ring; and sorry, i still not find why last trb in an event ring skipped by H/W according to the current logic. Thanks! Read the specs and the code. David -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] xhci: fix array index out of the bounds in function last_trb() and last_trb_on_last_seg()
On Mon, Dec 16, 2013 at 03:20:28PM -, David Laight wrote: I know all these difference clearly, inc_deq() is indeed a common function for different rings, but lasr_trb() last_trb_on_last_seg() inside it use different condition to determine the last trb in an event ring and an non-event ring; and sorry, i still not find why last trb in an event ring skipped by H/W according to the current logic. Thanks! Read the specs and the code. State your objections clearly. Based on my analysis, this patch will produce correct behavior with the event ring: http://marc.info/?l=linux-usbm=138697807827548w=2 Am I missing something? Sarah Sharp -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
