Hi,
Who is responsible person for applying patches to main tree?
This time I decide to send 2.5.5 patch version:
--- serial-255.c Thu Feb 28 19:24:47 2002
+++ ../serial-255.c Wed Feb 20 05:10:59 2002
@@ -2084,7 +2084,6 @@
unsigned int i,change_irq,change_port;
int retval = 0;
unsigned long new_port;
- unsigned long new_mem;
if (copy_from_user(&new_serial,new_info,sizeof(new_serial)))
return -EFAULT;
@@ -2094,7 +2093,6 @@
new_port = new_serial.port;
if (HIGH_BITS_OFFSET)
new_port += (unsigned long) new_serial.port_high <<
HIGH_BITS_OFFSET;
- new_mem = new_serial.iomem_base;
change_irq = new_serial.irq != state->irq;
change_port = (new_port != ((int) state->port)) ||
@@ -2136,7 +2134,6 @@
for (i = 0 ; i < NR_PORTS; i++)
if ((state != &rs_table[i]) &&
(rs_table[i].port == new_port) &&
- (rs_table[i].iomem_base == new_mem) &&
rs_table[i].type)
return -EADDRINUSE;
}
-------- Original Message --------
Subject: Serial.c Bug
Date: Wed, 14 Nov 2001 13:02:47 +0300
From: Roman Kurakin <[email protected]>
To: [email protected]
I have found a bug. It is in support of serial cards which uses
memory for
I/O insted of ports. I made a patch for serial.c and fix one place, but
probably the problem like this one could be somewhere else.
If you try to use setserial with such cards you will get "Address in use"
(-EADDRINUSE)
Best regards,
Kurakin Roman
Best regards,
Roman Kurakin
On Fri, Mar 01, 2002 at 4:19 AM, Roman Kurakin wrote:
>
> Who is responsible person for applying [serial driver] patches
> to main tree?
Hi Roman,
Well it's a little complicated. Russell King is the official serial driver
maintainer, at least for 2.5. He is very busy right now on a rewrite of the
serial driver subsystem that will eventually make things better in many ways
for writing support for new devices with serial nature.
He has (properly) backed away from trying to also support the existing
driver. There has been no willing victim to take on maintainer duties for
the existing driver since Ted Tso got busy with other things over a year
ago.
BTW, your patch is correct and, as you suspected, there are indeed other
ways that the existing driver is broken for memory mapped devices. My
favorite is the bug that causes kudzu to die with a null pointer dereference
during system initialization IFF there is a memory mapped serial card
present.
I have been trying to volunteer to ride the current driver into its sunset,
so as to (1) get my own changes in :-), and (2) call for the ignored patches
and help fix the known broken bits (be it known that since 2.4 is a "stable"
release, there are good ideas/enhancements that we simply should not do in
2.4) - without distracting Russell from his good work. The last word I
received was:
> The more help the better as always - providing you can co-ordinate
> with Russell.
I then asked Russell to set the rules for this co-ordination and no response
has been forthcoming. Perhaps he missed my question? So there is almost, but
not quite, somebody official to evaluate changes to the existing driver and
push the verified patches to Marcelo. I don't think it is time to abandon
the existing driver, just yet.
BTW, I monitor the abandoned linux-serial mailing list. If you post ignored
patches there I will be more likely to see them than on lkml. If and when
the people in charge say "go", I will start grinding on the existing serial
driver.
Best regards,
Ed Vance
----------------------------------------------------------------
Ed Vance [email protected]
Macrolink, Inc. 1500 N. Kellogg Dr Anaheim, CA 92807
----------------------------------------------------------------
-----Original Message-----
From: Roman Kurakin [mailto:[email protected]]
Sent: Friday, March 01, 2002 4:19 AM
To: [email protected]
Subject: Serial.c BUG 2.4.x-2.5x
Hi,
Who is responsible person for applying patches to main tree?
This time I decide to send 2.5.5 patch version:
--- serial-255.c Thu Feb 28 19:24:47 2002
+++ ../serial-255.c Wed Feb 20 05:10:59 2002
@@ -2084,7 +2084,6 @@
unsigned int i,change_irq,change_port;
int retval = 0;
unsigned long new_port;
- unsigned long new_mem;
if (copy_from_user(&new_serial,new_info,sizeof(new_serial)))
return -EFAULT;
@@ -2094,7 +2093,6 @@
new_port = new_serial.port;
if (HIGH_BITS_OFFSET)
new_port += (unsigned long) new_serial.port_high <<
HIGH_BITS_OFFSET;
- new_mem = new_serial.iomem_base;
change_irq = new_serial.irq != state->irq;
change_port = (new_port != ((int) state->port)) ||
@@ -2136,7 +2134,6 @@
for (i = 0 ; i < NR_PORTS; i++)
if ((state != &rs_table[i]) &&
(rs_table[i].port == new_port) &&
- (rs_table[i].iomem_base == new_mem) &&
rs_table[i].type)
return -EADDRINUSE;
}
-------- Original Message --------
Subject: Serial.c Bug
Date: Wed, 14 Nov 2001 13:02:47 +0300
From: Roman Kurakin <[email protected]>
To: [email protected]
I have found a bug. It is in support of serial cards which uses
memory for
I/O insted of ports. I made a patch for serial.c and fix one place, but
probably the problem like this one could be somewhere else.
If you try to use setserial with such cards you will get "Address in use"
(-EADDRINUSE)
Best regards,
Kurakin Roman
Best regards,
Roman Kurakin
On Fri, Mar 01, 2002 at 11:07:03AM -0800, Ed Vance wrote:
> On Fri, Mar 01, 2002 at 4:19 AM, Roman Kurakin wrote:
> >
> > Who is responsible person for applying [serial driver] patches
> > to main tree?
This particular bug has already been fixed in the rewrite, as I originally
said back on 14 November 2001.
The patch does fine for the most part, but I have two worries:
1. the possibilities of pushing through changes in the IO or memory space
by changing the other space at the same time. (ie, port = 1, iomem =
0xfe007c00 and you already have a line at port = 0, iomem = 0xfe007c00).
I delt with this properly using the resource management subsystem.
2. there seems to be a lack of security considerations for changing the
iomem address. (ie, changing the iomem address without CAP_SYS_ADMIN.
I added this as an extra check for change_port)
> I then asked Russell to set the rules for this co-ordination and no response
> has been forthcoming. Perhaps he missed my question?
I have a fair bit of email backed up at the moment, but I have been in
contact with Ted T'so recently. I won't say much more at the moment,
but should have something in a month or two. Until then I'd rather not
say too much publically.
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
Hi,
Russell King wrote:
>On Fri, Mar 01, 2002 at 11:07:03AM -0800, Ed Vance wrote:
>
>>On Fri, Mar 01, 2002 at 4:19 AM, Roman Kurakin wrote:
>>
>>> Who is responsible person for applying [serial driver] patches
>>> to main tree?
>>>
>
>This particular bug has already been fixed in the rewrite, as I originally
>said back on 14 November 2001.
>
I remember this, I thought some one responsible for updating current
version of the main tree.
Now I see the reason this didn't get into recent stable versions.
>The patch does fine for the most part, but I have two worries:
>
>1. the possibilities of pushing through changes in the IO or memory space
> by changing the other space at the same time. (ie, port = 1, iomem =
> 0xfe007c00 and you already have a line at port = 0, iomem = 0xfe007c00).
> I delt with this properly using the resource management subsystem.
>
I think such code could solve this problem ...
- (rs_table[i].port == new_port) &&
+ ((rs_table[i].port && rs_table[i].port == new_port) ||
+ ((rs_table[i].iomem_base && rs_table[i].iomem_base == new_mem)) &&
>2. there seems to be a lack of security considerations for changing the
> iomem address. (ie, changing the iomem address without CAP_SYS_ADMIN.
> I added this as an extra check for change_port)
>
And this one could be fixed with something like this (this is no a
patch, just an idea)
change_port = (new_port != ((int) state->port)) ||
(new_serial.hub6 != state->hub6);
+ change_mem = (new_mem != state->iomem_base)
if (!capable(CAP_SYS_ADMIN)) {
- if (change_irq || change_port ||
+ if (change_irq || change_port || change_mem
As I wrote I didn't check serial.c for all possible problems, I just
find one bug and suggested
the way it could be solved.
Best regards,
Roman Kurakin
>>I then asked Russell to set the rules for this co-ordination and no response
>>has been forthcoming. Perhaps he missed my question?
>>
>
>I have a fair bit of email backed up at the moment, but I have been in
>contact with Ted T'so recently. I won't say much more at the moment,
>but should have something in a month or two. Until then I'd rather not
>say too much publically.
>
Thanks Russell and Roman for your helpful input. I'll look at it today and
resubmit for further discussion.
Ed Vance
Roman Kurakin wrote:
> Russell King wrote:
>
> >The patch does fine for the most part, but I have two worries:
> >
> >1. the possibilities of pushing through changes in the IO or memory space
> > by changing the other space at the same time. (ie, port = 1, iomem =
> > 0xfe007c00 and you already have a line at port = 0, iomem =
0xfe007c00).
> > I delt with this properly using the resource management subsystem.
> >
> I think such code could solve this problem ...
>
> - (rs_table[i].port == new_port) &&
> + ((rs_table[i].port && rs_table[i].port ==
new_port) ||
> + ((rs_table[i].iomem_base &&
rs_table[i].iomem_base == new_mem)) &&
>
>
> >2. there seems to be a lack of security considerations for changing the
> > iomem address. (ie, changing the iomem address without CAP_SYS_ADMIN.
> > I added this as an extra check for change_port)
> >
> And this one could be fixed with something like this (this is no a
> patch, just an idea)
>
> change_port = (new_port != ((int) state->port)) ||
> (new_serial.hub6 != state->hub6);
> + change_mem = (new_mem != state->iomem_base)
>
> if (!capable(CAP_SYS_ADMIN)) {
> - if (change_irq || change_port ||
> + if (change_irq || change_port || change_mem
>
----------------------------------------------------------------
Ed Vance [email protected]
Macrolink, Inc. 1500 N. Kellogg Dr Anaheim, CA 92807
----------------------------------------------------------------
On Thu Mar 07, 2002, Roman Kurakin wrote:
>
> On Wed Mar 06, 2002, Russell King wrote:
> >
> >The patch does fine for the most part, but I have two worries:
> >
> >1. the possibilities of pushing through changes in the IO or memory space
> > by changing the other space at the same time. (ie, port = 1, iomem =
> > 0xfe007c00 and you already have a line at port = 0, iomem =
0xfe007c00).
> > I dealt with this properly using the resource management subsystem.
> >
> I think such code could solve this problem ...
>
> - (rs_table[i].port == new_port) &&
> + ((rs_table[i].port && rs_table[i].port == new_port) ||
> + ((rs_table[i].iomem_base && rs_table[i].iomem_base == new_mem))
&&
Indeed it would solve this problem, but I'm not sure there is a problem to
solve here. Have not found a case where ->port and ->iomem_base fields can
both be non-zero. If one of them is always zero then the previous patch hunk
in the "address in use" test at about line 2146 is well enough:
if ((state != &rs_table[i]) &&
(rs_table[i].port == new_port) &&
+ (rs_table[i].iomem_base == new_mem) &&
rs_table[i].type)
return -EADDRINUSE;
Assuming one of the two fields is always zero, demanding both to match for
the in use condition works anyway. If the non-zero field matches, then they
both must match. The following hunk at the bottom of function get_pci_port()
at about line 3931 seems to guarantee that they start out this way:
[[ The req struct is memset() to zero at about line 4009 in
function start_pci_pnp_board(). ]]
if (IS_PCI_REGION_IOPORT(dev, base_idx)) {
req->port = port;
if (HIGH_BITS_OFFSET)
req->port_high = port >> HIGH_BITS_OFFSET;
else
req->port_high = 0;
return 0;
}
req->io_type = SERIAL_IO_MEM;
req->iomem_base = ioremap(port, board->uart_offset);
req->iomem_reg_shift = board->reg_shift;
req->port = 0;
return 0;
Does anybody see a need to add the code anyway? Did I miss a lurker?
Best to all,
----------------------------------------------------------------
Ed Vance [email protected]
Macrolink, Inc. 1500 N. Kellogg Dr Anaheim, CA 92807
----------------------------------------------------------------