Re: 2.6.23-rc6-mm1

[Satyam Sharma]

Hi Greg,


On Tue, 18 Sep 2007, Greg KH wrote:
> 
> On Tue, Sep 18, 2007 at 03:04:48PM +0530, Satyam Sharma wrote:
> > 
> > But wait ... isn't that a statically-allocated kobject, which were
> > supposed to be "naughty" in the first place?
> 
> Yes it is, if you want to dynamically create it, please do.

Sorry for being late to reply, but do you still want such a patch (i.e.
convert static to dynamic allocation)?

I read elsewhere on this thread that you'd merge Kamalesh's patch and
fix it up to also use kobject_name() yourself. But it's a small/trivial
driver, so I think just converting it to dynamic allocation right now
itself (when we've noticed it already) is probably better (?)

[ BTW I don't see the fix in your git trees or quilt queue. So I'll
  make a patch on top of 2.6.23-rc6-mm1 itself. ]


Thanks,

Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: 2.6.23-rc6-mm1

[Satyam Sharma]

> -static volatile int kgdb_hwbreak_sstep[NR_CPUS];
> +volatile int kgdb_hwbreak_sstep[NR_CPUS];

That looks fishy to me. Why is it volatile-qualified? And does that
actually want to be a per-cpu var?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH -mm] iseries_veth: Kill unused variable

[Satyam Sharma]

drivers/net/iseries_veth.c: In function 'veth_transmit_to_many':
drivers/net/iseries_veth.c:1174: warning: unused variable 'port'

Signed-off-by: Satyam Sharma <[EMAIL PROTECTED]>

---

 drivers/net/iseries_veth.c |1 -
 1 file changed, 1 deletion(-)

diff -ruNp a/drivers/net/iseries_veth.c b/drivers/net/iseries_veth.c
--- a/drivers/net/iseries_veth.c2007-09-22 06:26:39.0 +0530
+++ b/drivers/net/iseries_veth.c2007-09-22 11:17:29.0 +0530
@@ -1171,7 +1171,6 @@ static void veth_transmit_to_many(struct
  HvLpIndexMap lpmask,
  struct net_device *dev)
 {
-   struct veth_port *port = (struct veth_port *) dev->priv;
int i, success, error;
 
success = error = 0;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH -mm] mv643xx_eth: Remove redundant multiple initialization

[Satyam Sharma]

Of ethtool_ops->get_stats_count and ethtool_ops->get_ethtool_stats.

Signed-off-by: Satyam Sharma <[EMAIL PROTECTED]>

---

 drivers/net/mv643xx_eth.c |2 --
 1 file changed, 2 deletions(-)

diff -ruNp a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
--- a/drivers/net/mv643xx_eth.c 2007-09-22 06:26:39.0 +0530
+++ b/drivers/net/mv643xx_eth.c 2007-09-22 13:15:41.0 +0530
@@ -2740,8 +2740,6 @@ static const struct ethtool_ops mv643xx_
.get_stats_count= mv643xx_get_stats_count,
.get_ethtool_stats  = mv643xx_get_ethtool_stats,
.get_strings= mv643xx_get_strings,
-   .get_stats_count= mv643xx_get_stats_count,
-   .get_ethtool_stats  = mv643xx_get_ethtool_stats,
.nway_reset = mv643xx_eth_nway_restart,
 };
 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH -mm] pasemi_mac: Build fix after recent netdev stats changes

[Satyam Sharma]

On Thu, 20 Sep 2007, Satyam Sharma wrote:
> 
> BTW ppc64_defconfig didn't quite like 2.6.23-rc6-mm1 either ...
> IIRC I got build failures in:

> drivers/net/pasemi_mac.c


[PATCH -mm] pasemi_mac: Build fix after recent netdev stats changes

Unbreak the following:

drivers/net/pasemi_mac.c: In function 'pasemi_mac_clean_rx':
drivers/net/pasemi_mac.c:533: error: 'dev' undeclared (first use in this 
function)
drivers/net/pasemi_mac.c:533: error: (Each undeclared identifier is reported 
only once
drivers/net/pasemi_mac.c:533: error: for each function it appears in.)

And remove an unused static function:

drivers/net/pasemi_mac.c: At top level:
drivers/net/pasemi_mac.c:89: warning: 'read_iob_reg' defined but not used

Signed-off-by: Satyam Sharma <[EMAIL PROTECTED]>

---

 drivers/net/pasemi_mac.c |9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff -ruNp a/drivers/net/pasemi_mac.c b/drivers/net/pasemi_mac.c
--- a/drivers/net/pasemi_mac.c  2007-09-22 06:26:39.0 +0530
+++ b/drivers/net/pasemi_mac.c  2007-09-22 13:03:04.0 +0530
@@ -85,11 +85,6 @@ MODULE_PARM_DESC(debug, "PA Semi MAC bit
 
 static struct pasdma_status *dma_status;
 
-static unsigned int read_iob_reg(struct pasemi_mac *mac, unsigned int reg)
-{
-   return in_le32(mac->iob_regs+reg);
-}
-
 static void write_iob_reg(struct pasemi_mac *mac, unsigned int reg,
  unsigned int val)
 {
@@ -530,8 +525,8 @@ static int pasemi_mac_clean_rx(struct pa
} else
skb->ip_summed = CHECKSUM_NONE;
 
-   dev->stats.rx_bytes += len;
-   dev->stats.rx_packets++;
+   mac->netdev->stats.rx_bytes += len;
+   mac->netdev->stats.rx_packets++;
 
skb->protocol = eth_type_trans(skb, mac->netdev);
netif_receive_skb(skb);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: 2.6.23-rc6-mm1: Build failures on ppc64_defconfig

[Satyam Sharma]

On Thu, 20 Sep 2007, Satyam Sharma wrote:
> 
> BTW ppc64_defconfig didn't quite like 2.6.23-rc6-mm1 either ...
> IIRC I got build failures in:

> drivers/net/spider_net.c

Fixing the above showed up another problem in another file of the
same driver (drivers/net/spider_net_ethtool.c)


[PATCH -mm] spider_net_ethtool: Keep up with recent netdev stats changes

Unbreak the following:

drivers/net/spider_net_ethtool.c: In function 'spider_net_get_ethtool_stats':
drivers/net/spider_net_ethtool.c:160: error: structure has no member named 
'netdev_stats'
drivers/net/spider_net_ethtool.c:161: error: structure has no member named 
'netdev_stats'
drivers/net/spider_net_ethtool.c:162: error: structure has no member named 
'netdev_stats'
drivers/net/spider_net_ethtool.c:163: error: structure has no member named 
'netdev_stats'
drivers/net/spider_net_ethtool.c:164: error: structure has no member named 
'netdev_stats'
drivers/net/spider_net_ethtool.c:165: error: structure has no member named 
'netdev_stats'
drivers/net/spider_net_ethtool.c:166: error: structure has no member named 
'netdev_stats'
make[2]: *** [drivers/net/spider_net_ethtool.o] Error 1

Also do another ARRAY_SIZE() cleanup while at it.

Signed-off-by: Satyam Sharma <[EMAIL PROTECTED]>

---

 drivers/net/spider_net_ethtool.c |   18 --
 1 file changed, 8 insertions(+), 10 deletions(-)

diff -ruNp a/drivers/net/spider_net_ethtool.c b/drivers/net/spider_net_ethtool.c
--- a/drivers/net/spider_net_ethtool.c  2007-09-22 06:26:39.0 +0530
+++ b/drivers/net/spider_net_ethtool.c  2007-09-22 12:43:51.0 +0530
@@ -28,8 +28,6 @@
 #include "spider_net.h"
 
 
-#define SPIDER_NET_NUM_STATS 13
-
 static struct {
const char str[ETH_GSTRING_LEN];
 } ethtool_stats_keys[] = {
@@ -149,7 +147,7 @@ spider_net_ethtool_get_ringparam(struct 
 
 static int spider_net_get_stats_count(struct net_device *netdev)
 {
-   return SPIDER_NET_NUM_STATS;
+   return ARRAY_SIZE(ethtool_stats_keys);
 }
 
 static void spider_net_get_ethtool_stats(struct net_device *netdev,
@@ -157,13 +155,13 @@ static void spider_net_get_ethtool_stats
 {
struct spider_net_card *card = netdev->priv;
 
-   data[0] = card->netdev_stats.tx_packets;
-   data[1] = card->netdev_stats.tx_bytes;
-   data[2] = card->netdev_stats.rx_packets;
-   data[3] = card->netdev_stats.rx_bytes;
-   data[4] = card->netdev_stats.tx_errors;
-   data[5] = card->netdev_stats.tx_dropped;
-   data[6] = card->netdev_stats.rx_dropped;
+   data[0] = netdev->stats.tx_packets;
+   data[1] = netdev->stats.tx_bytes;
+   data[2] = netdev->stats.rx_packets;
+   data[3] = netdev->stats.rx_bytes;
+   data[4] = netdev->stats.tx_errors;
+   data[5] = netdev->stats.tx_dropped;
+   data[6] = netdev->stats.rx_dropped;
data[7] = card->spider_stats.rx_desc_error;
data[8] = card->spider_stats.tx_timeouts;
data[9] = card->spider_stats.alloc_rx_skb_error;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: 2.6.23-rc6-mm1: Build failures on ppc64_defconfig

[Satyam Sharma]

On Thu, 20 Sep 2007, Satyam Sharma wrote:
> 
> BTW ppc64_defconfig didn't quite like 2.6.23-rc6-mm1 either ...
> IIRC I got build failures in:

> drivers/net/spider_net.c


[PATCH -mm] spider_net: Misc build fixes after recent netdev stats changes

Unbreak the following:

drivers/net/spider_net.c: In function 'spider_net_release_tx_chain':
drivers/net/spider_net.c:818: error: 'dev' undeclared (first use in this 
function)
drivers/net/spider_net.c:818: error: (Each undeclared identifier is reported 
only once
drivers/net/spider_net.c:818: error: for each function it appears in.)
drivers/net/spider_net.c: In function 'spider_net_xmit':
drivers/net/spider_net.c:922: error: 'dev' undeclared (first use in this 
function)
drivers/net/spider_net.c: In function 'spider_net_pass_skb_up':
drivers/net/spider_net.c:1018: error: 'dev' undeclared (first use in this 
function)
drivers/net/spider_net.c: In function 'spider_net_decode_one_descr':
drivers/net/spider_net.c:1215: error: 'dev' undeclared (first use in this 
function)
make[2]: *** [drivers/net/spider_net.o] Error 1

Signed-off-by: Satyam Sharma <[EMAIL PROTECTED]>

---

 drivers/net/spider_net.c |   24 +++-
 1 file changed, 11 insertions(+), 13 deletions(-)

diff -ruNp a/drivers/net/spider_net.c b/drivers/net/spider_net.c
--- a/drivers/net/spider_net.c  2007-09-22 06:26:39.0 +0530
+++ b/drivers/net/spider_net.c  2007-09-22 12:12:23.0 +0530
@@ -795,6 +795,7 @@ spider_net_set_low_watermark(struct spid
 static int
 spider_net_release_tx_chain(struct spider_net_card *card, int brutal)
 {
+   struct net_device *dev = card->netdev;
struct spider_net_descr_chain *chain = >tx_chain;
struct spider_net_descr *descr;
struct spider_net_hw_descr *hwdescr;
@@ -919,7 +920,7 @@ spider_net_xmit(struct sk_buff *skb, str
spider_net_release_tx_chain(card, 0);
 
if (spider_net_prepare_tx_descr(card, skb) != 0) {
-   dev->stats.tx_dropped++;
+   netdev->stats.tx_dropped++;
netif_stop_queue(netdev);
return NETDEV_TX_BUSY;
}
@@ -979,16 +980,12 @@ static void
 spider_net_pass_skb_up(struct spider_net_descr *descr,
   struct spider_net_card *card)
 {
-   struct spider_net_hw_descr *hwdescr= descr->hwdescr;
-   struct sk_buff *skb;
-   struct net_device *netdev;
-   u32 data_status, data_error;
-
-   data_status = hwdescr->data_status;
-   data_error = hwdescr->data_error;
-   netdev = card->netdev;
+   struct spider_net_hw_descr *hwdescr = descr->hwdescr;
+   struct sk_buff *skb = descr->skb;
+   struct net_device *netdev = card->netdev;
+   u32 data_status = hwdescr->data_status;
+   u32 data_error = hwdescr->data_error;
 
-   skb = descr->skb;
skb_put(skb, hwdescr->valid_size);
 
/* the card seems to add 2 bytes of junk in front
@@ -1015,8 +1012,8 @@ spider_net_pass_skb_up(struct spider_net
}
 
/* update netdevice statistics */
-   dev->stats.rx_packets++;
-   dev->stats.rx_bytes += skb->len;
+   netdev->stats.rx_packets++;
+   netdev->stats.rx_bytes += skb->len;
 
/* pass skb up to stack */
netif_receive_skb(skb);
@@ -1184,6 +1181,7 @@ static int spider_net_resync_tail_ptr(st
 static int
 spider_net_decode_one_descr(struct spider_net_card *card)
 {
+   struct net_device *dev = card->netdev;
struct spider_net_descr_chain *chain = >rx_chain;
struct spider_net_descr *descr = chain->tail;
struct spider_net_hw_descr *hwdescr = descr->hwdescr;
@@ -1210,7 +1208,7 @@ spider_net_decode_one_descr(struct spide
 (status == SPIDER_NET_DESCR_PROTECTION_ERROR) ||
 (status == SPIDER_NET_DESCR_FORCE_END) ) {
if (netif_msg_rx_err(card))
-   dev_err(>netdev->dev,
+   dev_err(>dev,
   "dropping RX descriptor with state %d\n", 
status);
dev->stats.rx_dropped++;
goto bad_desc;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: 2.6.23-rc6-mm1: Build failures on ppc64_defconfig

[Satyam Sharma]

On Thu, 20 Sep 2007, Satyam Sharma wrote:
> 
> BTW ppc64_defconfig didn't quite like 2.6.23-rc6-mm1 either ...
> IIRC I got build failures in:

> drivers/md/raid6int8.c

This turned out to be a gcc bug -- I was using an old cross-compiler.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: 2.6.23-rc6-mm1: Build failures on ppc64_defconfig

[Satyam Sharma]

On Thu, 20 Sep 2007, Satyam Sharma wrote:
> 
> BTW ppc64_defconfig didn't quite like 2.6.23-rc6-mm1 either ...
> IIRC I got build failures in:

> drivers/ata/pata_scc.c

http://lkml.org/lkml/2007/9/21/557
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: 2.6.23-rc6-mm1: Build failures on ppc64_defconfig

[Satyam Sharma]

On Thu, 20 Sep 2007, Satyam Sharma wrote:
 
 BTW ppc64_defconfig didn't quite like 2.6.23-rc6-mm1 either ...
 IIRC I got build failures in:

 drivers/ata/pata_scc.c

http://lkml.org/lkml/2007/9/21/557
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: 2.6.23-rc6-mm1: Build failures on ppc64_defconfig

[Satyam Sharma]

On Thu, 20 Sep 2007, Satyam Sharma wrote:
 
 BTW ppc64_defconfig didn't quite like 2.6.23-rc6-mm1 either ...
 IIRC I got build failures in:

 drivers/md/raid6int8.c

This turned out to be a gcc bug -- I was using an old cross-compiler.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: 2.6.23-rc6-mm1: Build failures on ppc64_defconfig

[Satyam Sharma]

On Thu, 20 Sep 2007, Satyam Sharma wrote:
 
 BTW ppc64_defconfig didn't quite like 2.6.23-rc6-mm1 either ...
 IIRC I got build failures in:

 drivers/net/spider_net.c


[PATCH -mm] spider_net: Misc build fixes after recent netdev stats changes

Unbreak the following:

drivers/net/spider_net.c: In function 'spider_net_release_tx_chain':
drivers/net/spider_net.c:818: error: 'dev' undeclared (first use in this 
function)
drivers/net/spider_net.c:818: error: (Each undeclared identifier is reported 
only once
drivers/net/spider_net.c:818: error: for each function it appears in.)
drivers/net/spider_net.c: In function 'spider_net_xmit':
drivers/net/spider_net.c:922: error: 'dev' undeclared (first use in this 
function)
drivers/net/spider_net.c: In function 'spider_net_pass_skb_up':
drivers/net/spider_net.c:1018: error: 'dev' undeclared (first use in this 
function)
drivers/net/spider_net.c: In function 'spider_net_decode_one_descr':
drivers/net/spider_net.c:1215: error: 'dev' undeclared (first use in this 
function)
make[2]: *** [drivers/net/spider_net.o] Error 1

Signed-off-by: Satyam Sharma [EMAIL PROTECTED]

---

 drivers/net/spider_net.c |   24 +++-
 1 file changed, 11 insertions(+), 13 deletions(-)

diff -ruNp a/drivers/net/spider_net.c b/drivers/net/spider_net.c
--- a/drivers/net/spider_net.c  2007-09-22 06:26:39.0 +0530
+++ b/drivers/net/spider_net.c  2007-09-22 12:12:23.0 +0530
@@ -795,6 +795,7 @@ spider_net_set_low_watermark(struct spid
 static int
 spider_net_release_tx_chain(struct spider_net_card *card, int brutal)
 {
+   struct net_device *dev = card-netdev;
struct spider_net_descr_chain *chain = card-tx_chain;
struct spider_net_descr *descr;
struct spider_net_hw_descr *hwdescr;
@@ -919,7 +920,7 @@ spider_net_xmit(struct sk_buff *skb, str
spider_net_release_tx_chain(card, 0);
 
if (spider_net_prepare_tx_descr(card, skb) != 0) {
-   dev-stats.tx_dropped++;
+   netdev-stats.tx_dropped++;
netif_stop_queue(netdev);
return NETDEV_TX_BUSY;
}
@@ -979,16 +980,12 @@ static void
 spider_net_pass_skb_up(struct spider_net_descr *descr,
   struct spider_net_card *card)
 {
-   struct spider_net_hw_descr *hwdescr= descr-hwdescr;
-   struct sk_buff *skb;
-   struct net_device *netdev;
-   u32 data_status, data_error;
-
-   data_status = hwdescr-data_status;
-   data_error = hwdescr-data_error;
-   netdev = card-netdev;
+   struct spider_net_hw_descr *hwdescr = descr-hwdescr;
+   struct sk_buff *skb = descr-skb;
+   struct net_device *netdev = card-netdev;
+   u32 data_status = hwdescr-data_status;
+   u32 data_error = hwdescr-data_error;
 
-   skb = descr-skb;
skb_put(skb, hwdescr-valid_size);
 
/* the card seems to add 2 bytes of junk in front
@@ -1015,8 +1012,8 @@ spider_net_pass_skb_up(struct spider_net
}
 
/* update netdevice statistics */
-   dev-stats.rx_packets++;
-   dev-stats.rx_bytes += skb-len;
+   netdev-stats.rx_packets++;
+   netdev-stats.rx_bytes += skb-len;
 
/* pass skb up to stack */
netif_receive_skb(skb);
@@ -1184,6 +1181,7 @@ static int spider_net_resync_tail_ptr(st
 static int
 spider_net_decode_one_descr(struct spider_net_card *card)
 {
+   struct net_device *dev = card-netdev;
struct spider_net_descr_chain *chain = card-rx_chain;
struct spider_net_descr *descr = chain-tail;
struct spider_net_hw_descr *hwdescr = descr-hwdescr;
@@ -1210,7 +1208,7 @@ spider_net_decode_one_descr(struct spide
 (status == SPIDER_NET_DESCR_PROTECTION_ERROR) ||
 (status == SPIDER_NET_DESCR_FORCE_END) ) {
if (netif_msg_rx_err(card))
-   dev_err(card-netdev-dev,
+   dev_err(dev-dev,
   dropping RX descriptor with state %d\n, 
status);
dev-stats.rx_dropped++;
goto bad_desc;
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: 2.6.23-rc6-mm1: Build failures on ppc64_defconfig

[Satyam Sharma]

On Thu, 20 Sep 2007, Satyam Sharma wrote:
 
 BTW ppc64_defconfig didn't quite like 2.6.23-rc6-mm1 either ...
 IIRC I got build failures in:

 drivers/net/spider_net.c

Fixing the above showed up another problem in another file of the
same driver (drivers/net/spider_net_ethtool.c)


[PATCH -mm] spider_net_ethtool: Keep up with recent netdev stats changes

Unbreak the following:

drivers/net/spider_net_ethtool.c: In function 'spider_net_get_ethtool_stats':
drivers/net/spider_net_ethtool.c:160: error: structure has no member named 
'netdev_stats'
drivers/net/spider_net_ethtool.c:161: error: structure has no member named 
'netdev_stats'
drivers/net/spider_net_ethtool.c:162: error: structure has no member named 
'netdev_stats'
drivers/net/spider_net_ethtool.c:163: error: structure has no member named 
'netdev_stats'
drivers/net/spider_net_ethtool.c:164: error: structure has no member named 
'netdev_stats'
drivers/net/spider_net_ethtool.c:165: error: structure has no member named 
'netdev_stats'
drivers/net/spider_net_ethtool.c:166: error: structure has no member named 
'netdev_stats'
make[2]: *** [drivers/net/spider_net_ethtool.o] Error 1

Also do another ARRAY_SIZE() cleanup while at it.

Signed-off-by: Satyam Sharma [EMAIL PROTECTED]

---

 drivers/net/spider_net_ethtool.c |   18 --
 1 file changed, 8 insertions(+), 10 deletions(-)

diff -ruNp a/drivers/net/spider_net_ethtool.c b/drivers/net/spider_net_ethtool.c
--- a/drivers/net/spider_net_ethtool.c  2007-09-22 06:26:39.0 +0530
+++ b/drivers/net/spider_net_ethtool.c  2007-09-22 12:43:51.0 +0530
@@ -28,8 +28,6 @@
 #include spider_net.h
 
 
-#define SPIDER_NET_NUM_STATS 13
-
 static struct {
const char str[ETH_GSTRING_LEN];
 } ethtool_stats_keys[] = {
@@ -149,7 +147,7 @@ spider_net_ethtool_get_ringparam(struct 
 
 static int spider_net_get_stats_count(struct net_device *netdev)
 {
-   return SPIDER_NET_NUM_STATS;
+   return ARRAY_SIZE(ethtool_stats_keys);
 }
 
 static void spider_net_get_ethtool_stats(struct net_device *netdev,
@@ -157,13 +155,13 @@ static void spider_net_get_ethtool_stats
 {
struct spider_net_card *card = netdev-priv;
 
-   data[0] = card-netdev_stats.tx_packets;
-   data[1] = card-netdev_stats.tx_bytes;
-   data[2] = card-netdev_stats.rx_packets;
-   data[3] = card-netdev_stats.rx_bytes;
-   data[4] = card-netdev_stats.tx_errors;
-   data[5] = card-netdev_stats.tx_dropped;
-   data[6] = card-netdev_stats.rx_dropped;
+   data[0] = netdev-stats.tx_packets;
+   data[1] = netdev-stats.tx_bytes;
+   data[2] = netdev-stats.rx_packets;
+   data[3] = netdev-stats.rx_bytes;
+   data[4] = netdev-stats.tx_errors;
+   data[5] = netdev-stats.tx_dropped;
+   data[6] = netdev-stats.rx_dropped;
data[7] = card-spider_stats.rx_desc_error;
data[8] = card-spider_stats.tx_timeouts;
data[9] = card-spider_stats.alloc_rx_skb_error;
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH -mm] pasemi_mac: Build fix after recent netdev stats changes

[Satyam Sharma]

On Thu, 20 Sep 2007, Satyam Sharma wrote:
 
 BTW ppc64_defconfig didn't quite like 2.6.23-rc6-mm1 either ...
 IIRC I got build failures in:

 drivers/net/pasemi_mac.c


[PATCH -mm] pasemi_mac: Build fix after recent netdev stats changes

Unbreak the following:

drivers/net/pasemi_mac.c: In function 'pasemi_mac_clean_rx':
drivers/net/pasemi_mac.c:533: error: 'dev' undeclared (first use in this 
function)
drivers/net/pasemi_mac.c:533: error: (Each undeclared identifier is reported 
only once
drivers/net/pasemi_mac.c:533: error: for each function it appears in.)

And remove an unused static function:

drivers/net/pasemi_mac.c: At top level:
drivers/net/pasemi_mac.c:89: warning: 'read_iob_reg' defined but not used

Signed-off-by: Satyam Sharma [EMAIL PROTECTED]

---

 drivers/net/pasemi_mac.c |9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff -ruNp a/drivers/net/pasemi_mac.c b/drivers/net/pasemi_mac.c
--- a/drivers/net/pasemi_mac.c  2007-09-22 06:26:39.0 +0530
+++ b/drivers/net/pasemi_mac.c  2007-09-22 13:03:04.0 +0530
@@ -85,11 +85,6 @@ MODULE_PARM_DESC(debug, PA Semi MAC bit
 
 static struct pasdma_status *dma_status;
 
-static unsigned int read_iob_reg(struct pasemi_mac *mac, unsigned int reg)
-{
-   return in_le32(mac-iob_regs+reg);
-}
-
 static void write_iob_reg(struct pasemi_mac *mac, unsigned int reg,
  unsigned int val)
 {
@@ -530,8 +525,8 @@ static int pasemi_mac_clean_rx(struct pa
} else
skb-ip_summed = CHECKSUM_NONE;
 
-   dev-stats.rx_bytes += len;
-   dev-stats.rx_packets++;
+   mac-netdev-stats.rx_bytes += len;
+   mac-netdev-stats.rx_packets++;
 
skb-protocol = eth_type_trans(skb, mac-netdev);
netif_receive_skb(skb);
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH -mm] mv643xx_eth: Remove redundant multiple initialization

[Satyam Sharma]

Of ethtool_ops-get_stats_count and ethtool_ops-get_ethtool_stats.

Signed-off-by: Satyam Sharma [EMAIL PROTECTED]

---

 drivers/net/mv643xx_eth.c |2 --
 1 file changed, 2 deletions(-)

diff -ruNp a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
--- a/drivers/net/mv643xx_eth.c 2007-09-22 06:26:39.0 +0530
+++ b/drivers/net/mv643xx_eth.c 2007-09-22 13:15:41.0 +0530
@@ -2740,8 +2740,6 @@ static const struct ethtool_ops mv643xx_
.get_stats_count= mv643xx_get_stats_count,
.get_ethtool_stats  = mv643xx_get_ethtool_stats,
.get_strings= mv643xx_get_strings,
-   .get_stats_count= mv643xx_get_stats_count,
-   .get_ethtool_stats  = mv643xx_get_ethtool_stats,
.nway_reset = mv643xx_eth_nway_restart,
 };
 
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH -mm] iseries_veth: Kill unused variable

[Satyam Sharma]

drivers/net/iseries_veth.c: In function 'veth_transmit_to_many':
drivers/net/iseries_veth.c:1174: warning: unused variable 'port'

Signed-off-by: Satyam Sharma [EMAIL PROTECTED]

---

 drivers/net/iseries_veth.c |1 -
 1 file changed, 1 deletion(-)

diff -ruNp a/drivers/net/iseries_veth.c b/drivers/net/iseries_veth.c
--- a/drivers/net/iseries_veth.c2007-09-22 06:26:39.0 +0530
+++ b/drivers/net/iseries_veth.c2007-09-22 11:17:29.0 +0530
@@ -1171,7 +1171,6 @@ static void veth_transmit_to_many(struct
  HvLpIndexMap lpmask,
  struct net_device *dev)
 {
-   struct veth_port *port = (struct veth_port *) dev-priv;
int i, success, error;
 
success = error = 0;
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: 2.6.23-rc6-mm1

[Satyam Sharma]

 -static volatile int kgdb_hwbreak_sstep[NR_CPUS];
 +volatile int kgdb_hwbreak_sstep[NR_CPUS];

That looks fishy to me. Why is it volatile-qualified? And does that
actually want to be a per-cpu var?
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: 2.6.23-rc6-mm1

[Satyam Sharma]

Hi Greg,


On Tue, 18 Sep 2007, Greg KH wrote:
 
 On Tue, Sep 18, 2007 at 03:04:48PM +0530, Satyam Sharma wrote:
  
  But wait ... isn't that a statically-allocated kobject, which were
  supposed to be naughty in the first place?
 
 Yes it is, if you want to dynamically create it, please do.

Sorry for being late to reply, but do you still want such a patch (i.e.
convert static to dynamic allocation)?

I read elsewhere on this thread that you'd merge Kamalesh's patch and
fix it up to also use kobject_name() yourself. But it's a small/trivial
driver, so I think just converting it to dynamic allocation right now
itself (when we've noticed it already) is probably better (?)

[ BTW I don't see the fix in your git trees or quilt queue. So I'll
  make a patch on top of 2.6.23-rc6-mm1 itself. ]


Thanks,

Satyam
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [34/50] i386: Fix argument signedness warnings

[Satyam Sharma]

Hi,


On Sat, 22 Sep 2007, Andi Kleen wrote:
> 
> From: Satyam Sharma <[EMAIL PROTECTED]>
> 
> 
> These build warnings:
> 
> In file included from include/asm/thread_info.h:16,
> from include/linux/thread_info.h:21,
> from include/linux/preempt.h:9,
> from include/linux/spinlock.h:49,
> from include/linux/vmalloc.h:4,
> from arch/i386/boot/compressed/misc.c:14:
> include/asm/processor.h: In function $B!F(Jcpuid_count$B!G(J:
   ^^   ^^
> include/asm/processor.h:615: warning: pointer targets in passing
> argument 1 of $B!F(Jnative_cpuid$B!G(J differ in signedness

> include/asm/processor.h:615: warning: pointer targets in passing
> argument 2 of $B!F(Jnative_cpuid$B!G(J differ in signedness

> include/asm/processor.h:615: warning: pointer targets in passing
> argument 3 of $B!F(Jnative_cpuid$B!G(J differ in signedness

> include/asm/processor.h:615: warning: pointer targets in passing
> argument 4 of $B!F(Jnative_cpuid$B!G(J differ in signedness
^^^^

Yikes. My bad, I had faulty (default) alpine settings (and a sad
combination of LANG=en_US.UTF-8) when I made and sent out that patch.
Please ensure that this finally gets committed in a somewhat saner and
more readable state to the tree.

Thanks,

Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] user.c: use kmem_cache_zalloc()

[Satyam Sharma]

On Fri, 21 Sep 2007, Andrew Morton wrote:
> 
> On Fri, 21 Sep 2007 13:39:06 +0400
> Alexey Dobriyan <[EMAIL PROTECTED]> wrote:
> 
> > Quite a few fields are zeroed during user_struct creation, so use
> > kmem_cache_zalloc() --  save a few lines and #ifdef. Also will help avoid
> >  #ifdef CONFIG_POSIX_MQUEUE in next patch.
> > 
> > Signed-off-by: Alexey Dobriyan <[EMAIL PROTECTED]>
> > ---
> > 
> >  kernel/user.c |   13 +
> >  1 file changed, 1 insertion(+), 12 deletions(-)
> > 
> > --- a/kernel/user.c
> > +++ b/kernel/user.c
> > @@ -129,21 +129,11 @@ struct user_struct * alloc_uid(struct user_namespace 
> > *ns, uid_t uid)
> > if (!up) {
> > struct user_struct *new;
> >  
> > -   new = kmem_cache_alloc(uid_cachep, GFP_KERNEL);
> > +   new = kmem_cache_zalloc(uid_cachep, GFP_KERNEL);
> > if (!new)
> > return NULL;
> > new->uid = uid;
> > atomic_set(>__count, 1);
> > -   atomic_set(>processes, 0);
> > -   atomic_set(>files, 0);
> > -   atomic_set(>sigpending, 0);
> > -#ifdef CONFIG_INOTIFY_USER
> > -   atomic_set(>inotify_watches, 0);
> > -   atomic_set(>inotify_devs, 0);
> > -#endif
> > -
> > -   new->mq_bytes = 0;
> > -   new->locked_shm = 0;
> 
> 
> This assumes that setting an atomic_t to the all-zeroes pattern is
> equivalent to atomic_set(v, 0).
> 
> This happens to be true for all present architectures, afaik.  But an
> architecture which has crappy primitives could quite legitimately implement
> its atomic_t as:
> 
> typedef struct {
>   int counter;
>   spinlock_t lock;
> } atomic_t;
> 
> in which case your assumption breaks.

Agreed, and this (implementing atomic ops using spinlocks) is already true
for the CRIS platform.

However, cris' implementation explicitly takes care to ensure that
atomic_t contains just a solitary int member, and no spinlock_t's
inside the atomic_t itself. [ include/asm-cris/arch-v32/atomic.h ]

Of course, that "128" limits scalability, so no more than 128 CPUs can be
executing atomic ops at any given instant of time, but admittedly I'm
getting anal here myself ... (but probably that's often perfectly the
right attitude to have too)


> So it's all a bit theoretical and a bit anal, and I'm sure we're making the
> same mistake in other places, but it's not a change I particularly like..

Hmm, it's borderline.

Such changes make text smaller (in terms of LOC as well vmlinux size).

But they also hurt grepping. Often we (at least I) want to grep for when
is a variable/struct member/etc getting initialized or getting
set/assigned to. Take this case, for example -- I bet it's important (for
overall logic) that those variables get initialized to zero. But *zalloc()
functions do that implicitly, so it wastes precious seconds or minutes of
developer time when grepping that code.

OTOH, we could make it standard practise to put a little comment on top
of such *zalloc() usages, explicitly enumerating the struct members that
that the *zalloc() is assumed to initialize to zero.




Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: 2.6.23-rc6-mm1: Build failure on ppc64 drivers/ata/pata_scc.c

[Satyam Sharma]

Hi,


On Thu, 20 Sep 2007, Alan Cox wrote:
> 
> On Thu, 20 Sep 2007 14:13:15 +0100
> [EMAIL PROTECTED] (Mel Gorman) wrote:
> 
> > PPC64 building allmodconfig fails to compile drivers/ata/pata_scc.c . It
> > doesn't show up on other arches because this driver is specific to the
> > architecture.
> > 
> > drivers/ata/pata_scc.c: In function `scc_bmdma_status'
> 
> Its not been updated to match the libata core changes. Try something like
> this. Whoever is maintaining it should also remove the prereset cable handling
> code and use the proper cable detect method.

It appears you forgot to fix scc_std_softreset() and one of its callsites
in scc_bdma_stop(). A complete patch is attempted below -- please review.


[PATCH -mm] pata_scc: Keep up with libata core API changes

Little fixlets, that the build started erroring / warning about:

drivers/ata/pata_scc.c: In function 'scc_bmdma_status':
drivers/ata/pata_scc.c:734: error: structure has no member named 'active_tag'
drivers/ata/pata_scc.c: In function 'scc_pata_prereset':
drivers/ata/pata_scc.c:866: warning: passing arg 1 of 'ata_std_prereset' from 
incompatible pointer type
drivers/ata/pata_scc.c: In function 'scc_error_handler':
drivers/ata/pata_scc.c:908: warning: passing arg 2 of 'ata_bmdma_drive_eh' from 
incompatible pointer type
drivers/ata/pata_scc.c:908: warning: passing arg 3 of 'ata_bmdma_drive_eh' from 
incompatible pointer type
drivers/ata/pata_scc.c:908: warning: passing arg 5 of 'ata_bmdma_drive_eh' from 
incompatible pointer type
make[2]: *** [drivers/ata/pata_scc.o] Error 1

Signed-off-by: Satyam Sharma <[EMAIL PROTECTED]>
Cc: Alan Cox <[EMAIL PROTECTED]>
Cc: Mel Gorman <[EMAIL PROTECTED]>

---

Andrew, this includes (supercedes) the previous two ones from Mel / Alan.

 drivers/ata/pata_scc.c |   21 -
 1 file changed, 12 insertions(+), 9 deletions(-)

diff -ruNp a/drivers/ata/pata_scc.c b/drivers/ata/pata_scc.c
--- a/drivers/ata/pata_scc.c2007-09-22 06:26:37.0 +0530
+++ b/drivers/ata/pata_scc.c2007-09-22 08:04:42.0 +0530
@@ -594,16 +594,17 @@ static unsigned int scc_bus_softreset(st
  * Note: Original code is ata_std_softreset().
  */
 
-static int scc_std_softreset (struct ata_port *ap, unsigned int *classes,
-  unsigned long deadline)
+static int scc_std_softreset(struct ata_link *link, unsigned int *classes,
+ unsigned long deadline)
 {
+   struct ata_port *ap = link->ap;
unsigned int slave_possible = ap->flags & ATA_FLAG_SLAVE_POSS;
unsigned int devmask = 0, err_mask;
u8 err;
 
DPRINTK("ENTER\n");
 
-   if (ata_link_offline(>link)) {
+   if (ata_link_offline(link)) {
classes[0] = ATA_DEV_NONE;
goto out;
}
@@ -692,7 +693,7 @@ static void scc_bmdma_stop (struct ata_q
printk(KERN_WARNING "%s: Internal Bus Error\n", 
DRV_NAME);
out_be32(bmid_base + SCC_DMA_INTST, INTSTS_BMSINT);
/* TBD: SW reset */
-   scc_std_softreset(ap, , deadline);
+   scc_std_softreset(>link, , deadline);
continue;
}
 
@@ -731,7 +732,7 @@ static u8 scc_bmdma_status (struct ata_p
void __iomem *mmio = ap->ioaddr.bmdma_addr;
u8 host_stat = in_be32(mmio + SCC_DMA_STATUS);
u32 int_status = in_be32(mmio + SCC_DMA_INTST);
-   struct ata_queued_cmd *qc = ata_qc_from_tag(ap, ap->active_tag);
+   struct ata_queued_cmd *qc = ata_qc_from_tag(ap, ap->link.active_tag);
static int retry = 0;
 
/* return if IOS_SS is cleared */
@@ -860,10 +861,10 @@ static void scc_bmdma_freeze (struct ata
  * @deadline: deadline jiffies for the operation
  */
 
-static int scc_pata_prereset(struct ata_port *ap, unsigned long deadline)
+static int scc_pata_prereset(struct ata_link *link, unsigned long deadline)
 {
-   ap->cbl = ATA_CBL_PATA80;
-   return ata_std_prereset(ap, deadline);
+   link->ap->cbl = ATA_CBL_PATA80;
+   return ata_std_prereset(link, deadline);
 }
 
 /**
@@ -874,8 +875,10 @@ static int scc_pata_prereset(struct ata_
  * Note: Original code is ata_std_postreset().
  */
 
-static void scc_std_postreset (struct ata_port *ap, unsigned int *classes)
+static void scc_std_postreset(struct ata_link *link, unsigned int *classes)
 {
+   struct ata_port *ap = link->ap;
+
DPRINTK("ENTER\n");
 
/* is double-select really necessary? */
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: 2.6.23-rc6-mm1: Build failure on ppc64 drivers/ata/pata_scc.c

[Satyam Sharma]

Hi,


On Thu, 20 Sep 2007, Alan Cox wrote:
 
 On Thu, 20 Sep 2007 14:13:15 +0100
 [EMAIL PROTECTED] (Mel Gorman) wrote:
 
  PPC64 building allmodconfig fails to compile drivers/ata/pata_scc.c . It
  doesn't show up on other arches because this driver is specific to the
  architecture.
  
  drivers/ata/pata_scc.c: In function `scc_bmdma_status'
 
 Its not been updated to match the libata core changes. Try something like
 this. Whoever is maintaining it should also remove the prereset cable handling
 code and use the proper cable detect method.

It appears you forgot to fix scc_std_softreset() and one of its callsites
in scc_bdma_stop(). A complete patch is attempted below -- please review.


[PATCH -mm] pata_scc: Keep up with libata core API changes

Little fixlets, that the build started erroring / warning about:

drivers/ata/pata_scc.c: In function 'scc_bmdma_status':
drivers/ata/pata_scc.c:734: error: structure has no member named 'active_tag'
drivers/ata/pata_scc.c: In function 'scc_pata_prereset':
drivers/ata/pata_scc.c:866: warning: passing arg 1 of 'ata_std_prereset' from 
incompatible pointer type
drivers/ata/pata_scc.c: In function 'scc_error_handler':
drivers/ata/pata_scc.c:908: warning: passing arg 2 of 'ata_bmdma_drive_eh' from 
incompatible pointer type
drivers/ata/pata_scc.c:908: warning: passing arg 3 of 'ata_bmdma_drive_eh' from 
incompatible pointer type
drivers/ata/pata_scc.c:908: warning: passing arg 5 of 'ata_bmdma_drive_eh' from 
incompatible pointer type
make[2]: *** [drivers/ata/pata_scc.o] Error 1

Signed-off-by: Satyam Sharma [EMAIL PROTECTED]
Cc: Alan Cox [EMAIL PROTECTED]
Cc: Mel Gorman [EMAIL PROTECTED]

---

Andrew, this includes (supercedes) the previous two ones from Mel / Alan.

 drivers/ata/pata_scc.c |   21 -
 1 file changed, 12 insertions(+), 9 deletions(-)

diff -ruNp a/drivers/ata/pata_scc.c b/drivers/ata/pata_scc.c
--- a/drivers/ata/pata_scc.c2007-09-22 06:26:37.0 +0530
+++ b/drivers/ata/pata_scc.c2007-09-22 08:04:42.0 +0530
@@ -594,16 +594,17 @@ static unsigned int scc_bus_softreset(st
  * Note: Original code is ata_std_softreset().
  */
 
-static int scc_std_softreset (struct ata_port *ap, unsigned int *classes,
-  unsigned long deadline)
+static int scc_std_softreset(struct ata_link *link, unsigned int *classes,
+ unsigned long deadline)
 {
+   struct ata_port *ap = link-ap;
unsigned int slave_possible = ap-flags  ATA_FLAG_SLAVE_POSS;
unsigned int devmask = 0, err_mask;
u8 err;
 
DPRINTK(ENTER\n);
 
-   if (ata_link_offline(ap-link)) {
+   if (ata_link_offline(link)) {
classes[0] = ATA_DEV_NONE;
goto out;
}
@@ -692,7 +693,7 @@ static void scc_bmdma_stop (struct ata_q
printk(KERN_WARNING %s: Internal Bus Error\n, 
DRV_NAME);
out_be32(bmid_base + SCC_DMA_INTST, INTSTS_BMSINT);
/* TBD: SW reset */
-   scc_std_softreset(ap, classes, deadline);
+   scc_std_softreset(ap-link, classes, deadline);
continue;
}
 
@@ -731,7 +732,7 @@ static u8 scc_bmdma_status (struct ata_p
void __iomem *mmio = ap-ioaddr.bmdma_addr;
u8 host_stat = in_be32(mmio + SCC_DMA_STATUS);
u32 int_status = in_be32(mmio + SCC_DMA_INTST);
-   struct ata_queued_cmd *qc = ata_qc_from_tag(ap, ap-active_tag);
+   struct ata_queued_cmd *qc = ata_qc_from_tag(ap, ap-link.active_tag);
static int retry = 0;
 
/* return if IOS_SS is cleared */
@@ -860,10 +861,10 @@ static void scc_bmdma_freeze (struct ata
  * @deadline: deadline jiffies for the operation
  */
 
-static int scc_pata_prereset(struct ata_port *ap, unsigned long deadline)
+static int scc_pata_prereset(struct ata_link *link, unsigned long deadline)
 {
-   ap-cbl = ATA_CBL_PATA80;
-   return ata_std_prereset(ap, deadline);
+   link-ap-cbl = ATA_CBL_PATA80;
+   return ata_std_prereset(link, deadline);
 }
 
 /**
@@ -874,8 +875,10 @@ static int scc_pata_prereset(struct ata_
  * Note: Original code is ata_std_postreset().
  */
 
-static void scc_std_postreset (struct ata_port *ap, unsigned int *classes)
+static void scc_std_postreset(struct ata_link *link, unsigned int *classes)
 {
+   struct ata_port *ap = link-ap;
+
DPRINTK(ENTER\n);
 
/* is double-select really necessary? */
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] user.c: use kmem_cache_zalloc()

[Satyam Sharma]

On Fri, 21 Sep 2007, Andrew Morton wrote:
 
 On Fri, 21 Sep 2007 13:39:06 +0400
 Alexey Dobriyan [EMAIL PROTECTED] wrote:
 
  Quite a few fields are zeroed during user_struct creation, so use
  kmem_cache_zalloc() --  save a few lines and #ifdef. Also will help avoid
   #ifdef CONFIG_POSIX_MQUEUE in next patch.
  
  Signed-off-by: Alexey Dobriyan [EMAIL PROTECTED]
  ---
  
   kernel/user.c |   13 +
   1 file changed, 1 insertion(+), 12 deletions(-)
  
  --- a/kernel/user.c
  +++ b/kernel/user.c
  @@ -129,21 +129,11 @@ struct user_struct * alloc_uid(struct user_namespace 
  *ns, uid_t uid)
  if (!up) {
  struct user_struct *new;
   
  -   new = kmem_cache_alloc(uid_cachep, GFP_KERNEL);
  +   new = kmem_cache_zalloc(uid_cachep, GFP_KERNEL);
  if (!new)
  return NULL;
  new-uid = uid;
  atomic_set(new-__count, 1);
  -   atomic_set(new-processes, 0);
  -   atomic_set(new-files, 0);
  -   atomic_set(new-sigpending, 0);
  -#ifdef CONFIG_INOTIFY_USER
  -   atomic_set(new-inotify_watches, 0);
  -   atomic_set(new-inotify_devs, 0);
  -#endif
  -
  -   new-mq_bytes = 0;
  -   new-locked_shm = 0;
 
 
 This assumes that setting an atomic_t to the all-zeroes pattern is
 equivalent to atomic_set(v, 0).
 
 This happens to be true for all present architectures, afaik.  But an
 architecture which has crappy primitives could quite legitimately implement
 its atomic_t as:
 
 typedef struct {
   int counter;
   spinlock_t lock;
 } atomic_t;
 
 in which case your assumption breaks.

Agreed, and this (implementing atomic ops using spinlocks) is already true
for the CRIS platform.

However, cris' implementation explicitly takes care to ensure that
atomic_t contains just a solitary int member, and no spinlock_t's
inside the atomic_t itself. [ include/asm-cris/arch-v32/atomic.h ]

Of course, that 128 limits scalability, so no more than 128 CPUs can be
executing atomic ops at any given instant of time, but admittedly I'm
getting anal here myself ... (but probably that's often perfectly the
right attitude to have too)


 So it's all a bit theoretical and a bit anal, and I'm sure we're making the
 same mistake in other places, but it's not a change I particularly like..

Hmm, it's borderline.

Such changes make text smaller (in terms of LOC as well vmlinux size).

But they also hurt grepping. Often we (at least I) want to grep for when
is a variable/struct member/etc getting initialized or getting
set/assigned to. Take this case, for example -- I bet it's important (for
overall logic) that those variables get initialized to zero. But *zalloc()
functions do that implicitly, so it wastes precious seconds or minutes of
developer time when grepping that code.

OTOH, we could make it standard practise to put a little comment on top
of such *zalloc() usages, explicitly enumerating the struct members that
that the *zalloc() is assumed to initialize to zero.

runs away


Satyam
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [34/50] i386: Fix argument signedness warnings

[Satyam Sharma]

Hi,


On Sat, 22 Sep 2007, Andi Kleen wrote:
 
 From: Satyam Sharma [EMAIL PROTECTED]
 
 
 These build warnings:
 
 In file included from include/asm/thread_info.h:16,
 from include/linux/thread_info.h:21,
 from include/linux/preempt.h:9,
 from include/linux/spinlock.h:49,
 from include/linux/vmalloc.h:4,
 from arch/i386/boot/compressed/misc.c:14:
 include/asm/processor.h: In function $B!F(Jcpuid_count$B!G(J:
   ^^   ^^
 include/asm/processor.h:615: warning: pointer targets in passing
 argument 1 of $B!F(Jnative_cpuid$B!G(J differ in signedness

 include/asm/processor.h:615: warning: pointer targets in passing
 argument 2 of $B!F(Jnative_cpuid$B!G(J differ in signedness

 include/asm/processor.h:615: warning: pointer targets in passing
 argument 3 of $B!F(Jnative_cpuid$B!G(J differ in signedness

 include/asm/processor.h:615: warning: pointer targets in passing
 argument 4 of $B!F(Jnative_cpuid$B!G(J differ in signedness
^^^^

Yikes. My bad, I had faulty (default) alpine settings (and a sad
combination of LANG=en_US.UTF-8) when I made and sent out that patch.
Please ensure that this finally gets committed in a somewhat saner and
more readable state to the tree.

Thanks,

Satyam
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] Time to make CONFIG_PARAVIRT non-experimental.

[Satyam Sharma]

On Thu, 20 Sep 2007, Charles N Wyble wrote:
> 
> Satyam Sharma wrote:
> > 
> > On Tue, 18 Sep 2007, Charles N Wyble wrote:
> >> Andi Kleen wrote:
> >>> Besides it's bad taste and taste is very important.
> >> Well it's bad taste for you (one person).
> > 
> > FWIW, my opinion is the same as Andi's here. Please, let's avoid this
> > disease -- unless *absolutely* required, stuff shouldn't be "default y".
> 
> I am curious why you think it shouldn't be default why? Bad taste? Can
> you provide data about how it will harm things?

Clear CONFIG_PARAVIRT from your .config and try "make oldconfig", and find
out for yourself :-)

You'll end up having to answer questions for all other config options
below it, that you never cared about, that you didn't know even existed.
You'd obviously (I would, at least) feel annoyed by it all ... this is
almost a regression :-)

And for those who automate the oldconfig process using "yes", well, this
"default y" *will* end up introducing a kernel text size regression --
all those config options they never had earlier, suddenly got enabled.

Besides, like Andi said, selecting or enabling CONFIG_PARAVIRT is quite
pointless in the first place -- it's just a menuconfig symbol, not a
"real" kconfig symbol that _actually_ controls code in the Makefiles.


Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] Time to make CONFIG_PARAVIRT non-experimental.

[Satyam Sharma]

On Tue, 18 Sep 2007, Charles N Wyble wrote:
> 
> Andi Kleen wrote:
> > 
> > Besides it's bad taste and taste is very important.
> 
> Well it's bad taste for you (one person).

FWIW, my opinion is the same as Andi's here. Please, let's avoid this
disease -- unless *absolutely* required, stuff shouldn't be "default y".
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

[Satyam Sharma]

On Thu, 20 Sep 2007, Maciej W. Rozycki wrote:
> 
>  Perhaps preinitialising to an error value such as -EINVAL would be of
> more sense.  This way any error paths lacking initialisation are still
> reported as errors, even though the classification might be wrong.

Eeee ... at least I wouldn't prefer that. Why not simply use the
"int x = x;" trick (which is what uninitialized_var() does) -- it shuts
up the warning, and does *nothing* else. The bug will not be hidden, if
there's bad misbehaviour happening due to the bug, it will continue to
happen that way -- thus bringing our attention to it. Pre-initializing
to -EINVAL (or whatever) has the problem that when the bug actually
triggers, something unrelated might happen higher up the callchain, and
we'd be scratching our heads in a "why are we getting a -EINVAL here?"
kind of way ... worse still, we might think that this was _really_ an
EINVAL and go about debugging it ...

Plus, pre-initializing to -EINVAL (or even 0) will waste some bytes in
kernel text size, but no such overhead with uninitialized_var() :-)


Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

[Satyam Sharma]

On Thu, 20 Sep 2007, Markus Gothe wrote:
> 
> GCC 4.1.2 has been stable for a long time now, maybe you better
> upgrade your binutils instead...

I'd been using 4.2.1 -- I don't want to downgrade to 4.1.2. (btw from
the discussion on gcc's bugzilla it appears the bug wasn't resolved
in 4.1.2 either?)

Satyam

> Satyam Sharma wrote:
> > Hi Maciej,
> >
> >
> > On Thu, 20 Sep 2007, Maciej W. Rozycki wrote:
> > > 
> > > On Wed, 19 Sep 2007, Andrew Morton wrote:
> > > > 
> > > > This initialisation to zero is not good.
> > > > 
> > > > Because if some error-path code forgot to do `err = -EFOO' then
> > > > probe() will return zero and the driver will leave things in
> > > > half-initialised state and will then proceed as if things had
> > > > succeeded.  It will crash.
> > > 
> > > GCC used to complain: "`foo' might be used uninitialized..." and
> > > this is the usual cure; let me see if this not the case anymore
> > > (I have 4.1.2).
> >
> > Even so, initializing to zero isn't quite good. You could use the
> > uninitialized_var() (once you've confirmed that the warning is
> > bogus). However, some maintainers may still nack
> > uninitialized_var() usage, quite legitimately.
> >
> >
> > > > So it's better to leave this local uninitialised, because we
> > > > really want to get that compiler warning if someone forgot to
> > > > set the return value.
> > > Yes of course, barring the issue mentioned.  Note the message
> > > above is not the same as: "`foo' is used uninitialized..." that
> > > would be reported in the case which you are concerned of.
> >
> > Firstly, "may be used uninitialized" can still be a bug.
> >
> > Secondly, latest gcc is *horribly* buggy (and has been so for last
> > several releases including 4.1, 4.2 and 4.3 -- 3.x was good). See:
> >
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33327
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501
> >
> > We'd been hurling all sorts of abuses on gcc for quite long (when
> > it fails to detect these "false positive" cases), but now, it turns
> > out it is quite easy to write *genuinely* buggy code that still
> > won't get any warnings, neither the "is used" nor "may be used"
> > one!
> >
> > In short, there are three ways to fix these false positive
> > warnings:
> >
> > 1. Do nothing, there are enough "uninitialized variable" warnings
> > anyway, and hopefully, one day GCC would clean up its act.
> >
> > 2. Use uninitialized_var() to shut it up (only if it's genuinely
> > bogus).
> >
> > 3. Do something like the following legendary patch [1]:
> >
> > http://kegel.com/crosstool/crosstool-0.43/patches/linux-2.6.11.3/arch_alpha_kernel_srcons.patch
> >
> >
> > i.e., explicitly change the structure/logic of the function to make
> > it obvious enough to gcc that the variable will not be used
> > uninitialized.
> >
> >
> > Satyam
> >
> > [1] That was a funny case -- the alpha linux maintainer is also a
> > gcc maintainer. Alpha even sets -Werror, so either he had to fix
> > the kernel code that produced the warning, or go fix GCC to not
> > warn about it -- he chose the former :-)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: error from checkpatch.pl version 0.10

[Satyam Sharma]

On Thu, 20 Sep 2007, Tetsuo Handa wrote:
> 
> I checked my patch using checkpatch.pl version 0.10
> and I got the following error.
> 
>   ERROR: need consistent spacing around '*' (ctx:WxV)
>   #2334: FILE: security/tomoyo/common.c:2306:
>   +static unsigned int tmy_poll(struct file *file, poll_table *wait)
>   ^

Looks like a checkpatch.pl bug to me -- that was nothing to warn about.


> What action should I take?
> Ignore this error because "poll_table" is used everywhere?
> Replace "poll_table" with "struct poll_table_struct" according to
> definition of "poll_table"?

Yeah, this would be better to do anyway (and rename poll_table_struct
to just poll_table).

>   typedef struct poll_table_struct {
>   poll_queue_proc qproc;
>   } poll_table;

So:

struct poll_table {
poll_queue_proc qproc;
};

In general the kernel's codingstyle consensus is to avoid adding typedefs.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

[Satyam Sharma]

Hi Maciej,


On Thu, 20 Sep 2007, Maciej W. Rozycki wrote:
> 
> On Wed, 19 Sep 2007, Andrew Morton wrote:
> 
> > > patch-mips-2.6.23-rc5-20070904-pmag-ba-err-2
> > > diff -up --recursive --new-file 
> > > linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c 
> > > linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c
> > > --- linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c   
> > > 2007-02-21 05:56:47.0 +
> > > +++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c 
> > > 2007-09-18 10:56:51.0 +
> > > @@ -147,16 +147,23 @@ static int __init pmagbafb_probe(struct 
> > >   resource_size_t start, len;
> > >   struct fb_info *info;
> > >   struct pmagbafb_par *par;
> > > + int err = 0;
> > 
> > This initialisation to zero is not good.
> > 
> > Because if some error-path code forgot to do `err = -EFOO' then probe()
> > will return zero and the driver will leave things in half-initialised state
> > and will then proceed as if things had succeeded.  It will crash.
> 
>  GCC used to complain: "`foo' might be used uninitialized..." and this is 
> the usual cure; let me see if this not the case anymore (I have 4.1.2).

Even so, initializing to zero isn't quite good. You could use the
uninitialized_var() (once you've confirmed that the warning is bogus).
However, some maintainers may still nack uninitialized_var() usage,
quite legitimately.


> > So it's better to leave this local uninitialised, because we really want to
> > get that compiler warning if someone forgot to set the return value.
> 
>  Yes of course, barring the issue mentioned.  Note the message above is 
> not the same as: "`foo' is used uninitialized..." that would be reported 
> in the case which you are concerned of.

Firstly, "may be used uninitialized" can still be a bug.

Secondly, latest gcc is *horribly* buggy (and has been so for last several
releases including 4.1, 4.2 and 4.3 -- 3.x was good). See:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33327
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501

We'd been hurling all sorts of abuses on gcc for quite long (when it fails
to detect these "false positive" cases), but now, it turns out it is quite
easy to write *genuinely* buggy code that still won't get any warnings,
neither the "is used" nor "may be used" one!

In short, there are three ways to fix these false positive warnings:

1. Do nothing, there are enough "uninitialized variable" warnings anyway,
   and hopefully, one day GCC would clean up its act.

2. Use uninitialized_var() to shut it up (only if it's genuinely bogus).

3. Do something like the following legendary patch [1]:

http://kegel.com/crosstool/crosstool-0.43/patches/linux-2.6.11.3/arch_alpha_kernel_srcons.patch

i.e., explicitly change the structure/logic of the function to make it
obvious enough to gcc that the variable will not be used uninitialized.


Satyam

[1] That was a funny case -- the alpha linux maintainer is also a gcc
maintainer. Alpha even sets -Werror, so either he had to fix the
kernel code that produced the warning, or go fix GCC to not warn
about it -- he chose the former :-)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

2.6.23-rc6-mm1: Build failures on ppc64_defconfig

[Satyam Sharma]

On Thu, 20 Sep 2007, Mel Gorman wrote:
> 
> allmodconfig on ppc64 fails to build with the following error

BTW ppc64_defconfig didn't quite like 2.6.23-rc6-mm1 either ...
IIRC I got build failures in:

drivers/ata/pata_scc.c
drivers/md/raid6int8.c
drivers/net/spider_net.c
drivers/net/pasemi_mac.c
drivers/pci/hotplug/rpadlpar_sysfs.c

I was in a hurry so didn't record the errors, just noted down the files
and disabled them from .config and continued building ... I'll get back
to fixing these up tonight (if you don't do that by then already).


Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [UFS] fs/ufs/super.c misreads the file system state

[Satyam Sharma]

On Wed, 19 Sep 2007, Leonid Kalev wrote:
> 
> ufs_get_fs_state() needs the file system type to read the state from the
> correct place in the superblock. It takes the type from UFS_SB(sb)->s_flags,
> but that value is stored after the first call to ufs_get_fs_state(). The patch
> below moves the assignment of s_flags up, before the first call to
> ufs_get_fs_state().
> 
> The patch is against linux-2.6.23-rc6-git7, but it applies (with offset) to
> 2.6.22 and to earlier versions as well. It has been tested on the Solaris
> flavor of UFS (ufstype=sunx86) - with this change, the file system can be used
> in read-write mode.

The same fix is already in 2.6.23-rc6-mm1 (don't know if it's lined up
for .23 though, I think it should be).


Satyam

PS: All my mails to Evgeniy Dushistov never reach him. The mail.ru server
is absolutely *evil* and simply rejects all mails unless the sender
is already whitelisted by the reciever (!)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/4] [-mm patch] use the existing offsetof().

[Satyam Sharma]

On Thu, 20 Sep 2007, Ken'ichi Ohmichi wrote:
> 
> [PATCH 6/4] [-mm patch] use the existing offsetof().
>  It is better that offsetof() is used for VMCOREINFO_OFFSET().
>  This idea is Joe Perches's.
> 
> 
> 
> Thanks
> Ken'ichi Ohmichi 
> 
> ---
> Signed-off-by: Joe Perches <[EMAIL PROTECTED]>

Same deal here ...


> Signed-off-by: Ken'ichi Ohmichi <[EMAIL PROTECTED]>
> ---
> diff -rpuN a/include/linux/kexec.h b/include/linux/kexec.h
> --- a/include/linux/kexec.h   2007-09-18 15:23:22.0 +0900
> +++ b/include/linux/kexec.h   2007-09-18 15:27:29.0 +0900
> @@ -137,7 +137,7 @@ unsigned long paddr_vmcoreinfo_note(void
> (unsigned long)sizeof(struct name))
>  #define VMCOREINFO_OFFSET(name, field) \
>   vmcoreinfo_append_str("OFFSET(%s.%s)=%lu\n", #name, #field, \
> -   (unsigned long)&(((struct name *)0)->field))
> +   (unsigned long)offsetof(struct name, field))
>  #define VMCOREINFO_LENGTH(name, value) \
>   vmcoreinfo_append_str("LENGTH(%s)=%lu\n", #name, (unsigned long)value)

... and here. Use %zu and lose the casts.

BTW I don't think these macros are such a big win in readability or
code clarity anyway. And I noticed that there are still some open
calls to vmcoreinfo_append_str() in kexec.c (such as for the OS_RELEASE
case) which you haven't macro-ized ... we end up having code that looks
like:

vmcoreinfo_append_str(...);
...

...
VMCOREINFO_OFFSET(...);
...

and it's not even obvious (unless you follow on to the definition of the
later macro) that the above two are *exactly* the same. So you could also
consider just losing the macros altogether.


Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 5/4] [-mm patch] Rename macros returning the size.

[Satyam Sharma]

Hi,


On Thu, 20 Sep 2007, Ken'ichi Ohmichi wrote:
> 
> [PATCH 5/4] [-mm patch] Rename macros returning the size.
>  The #define SIZE() should be renamed STRUCT_SIZE() since it's always 
>  returning the size of the struct with a given name.  This would allow 
>  TYPEDEF_SIZE() to simply become SIZE() since it need not be used 
>  exclusively for typedefs. This idea is David Rientjes's.
>  http://www.ussg.iu.edu/hypermail/linux/kernel/0709.1/1964.html
> 
> Thanks
> Ken'ichi Ohmichi
> 
> ---
> Signed-off-by: David Rientjes <[EMAIL PROTECTED]>

Hmm, I think adding a s-o-b line for David here isn't quite correct.
When someone reviews a patch and gives a suggestion, you only need to
copy him on the next iteration (and he may ack it or whatever, if he
wants) -- but adding a s-o-b line like that ends up (incorrectly)
denoting that he came between the author-to-git-commit chain ...


> Signed-off-by: Ken'ichi Ohmichi <[EMAIL PROTECTED]>

> --- a/include/linux/kexec.h   2007-09-18 15:22:19.0 +0900
> +++ b/include/linux/kexec.h   2007-09-18 15:23:22.0 +0900
> @@ -131,10 +131,10 @@ unsigned long paddr_vmcoreinfo_note(void
>   vmcoreinfo_append_str("SYMBOL(%s)=%lx\n", #name, (unsigned long))
>  #define VMCOREINFO_SIZE(name) \
>   vmcoreinfo_append_str("SIZE(%s)=%lu\n", #name, \
> -   (unsigned long)sizeof(struct name))
> -#define VMCOREINFO_TYPEDEF_SIZE(name) \
> - vmcoreinfo_append_str("SIZE(%s)=%lu\n", #name, \
> (unsigned long)sizeof(name))
> +#define VMCOREINFO_STRUCT_SIZE(name) \
> + vmcoreinfo_append_str("SIZE(%s)=%lu\n", #name, \
> +   (unsigned long)sizeof(struct name))
>  #define VMCOREINFO_OFFSET(name, field) \
>   vmcoreinfo_append_str("OFFSET(%s.%s)=%lu\n", #name, #field, \
> (unsigned long)&(((struct name *)0)->field))

Please use %zu and lose all the ugly (unsigned long) casts here.

Otherwise it looks good,

Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NFS4 authentification / fsuid

[Satyam Sharma]

Hi Valdis,


On Wed, 19 Sep 2007, [EMAIL PROTECTED] wrote:
> 
> On Wed, 19 Sep 2007 01:16:28 EDT, Kyle Moffett said:
> 
> > I am assuming that if the laptop has sufficiently important data on  
> > it to warrant the above steps then I am also clueful enough to:
> >(A)  Not carry the laptop around unsecured areas,
> >(B)  Keep a close enough eye on it and be aware that it's gone by  
> > the time they get to step 2, OR
> >(C)  Pay somebody to build me a better physical chassis for my laptop
> 
> Building a better chassis can be a challenge when the threat model really
> *does* include attacks by a well-funded TLA.
> 
> http://www.epic.org/crypto/scarfo/murch_aff.pdf

Thanks for the link! And this was ... 1999 (!) Probably goes without
saying that physical hacking tricks have only become even more refined/
perfected by now :-)


> The FBI did an *initial* entry to survey the hardware, and then a total of
> *five* other entries before they actually installed it.  Note the technical
> and legal requirements required on the KLS (it had to, among other things,
> capture PGP passphrases but *not* anything that was typed online).


Thanks,

Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NFS4 authentification / fsuid

[Satyam Sharma]

On Wed, 19 Sep 2007, J. Bruce Fields wrote:
> 
> Uh, is there somebody else that feels they're being enlightened by this
> discussion?

Ok, probably I got a bit too harsh with Kyle there. But what I don't
understand is why is it so hard for someone to accept they're wrong
on this list, thank the other person and just move on ?!

Even when you've explained an attack that wasn't considered, you've
explained why the assumptions of the scheme were wrong, you've mentioned
previous precedents, and *technically* refuted the other person's claims.
Still, people feel obliged to *stick* to their (wrong) positions, subtly
"shift" their argument (using new adjectives or qualifiers), whatever,
just as long as they don't have to accept they were, simply, wrong.

That just sucks.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NFS4 authentification / fsuid

[Satyam Sharma]

On Wed, 19 Sep 2007, J. Bruce Fields wrote:
 
 Uh, is there somebody else that feels they're being enlightened by this
 discussion?

Ok, probably I got a bit too harsh with Kyle there. But what I don't
understand is why is it so hard for someone to accept they're wrong
on this list, thank the other person and just move on ?!

Even when you've explained an attack that wasn't considered, you've
explained why the assumptions of the scheme were wrong, you've mentioned
previous precedents, and *technically* refuted the other person's claims.
Still, people feel obliged to *stick* to their (wrong) positions, subtly
shift their argument (using new adjectives or qualifiers), whatever,
just as long as they don't have to accept they were, simply, wrong.

That just sucks.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NFS4 authentification / fsuid

[Satyam Sharma]

Hi Valdis,


On Wed, 19 Sep 2007, [EMAIL PROTECTED] wrote:
 
 On Wed, 19 Sep 2007 01:16:28 EDT, Kyle Moffett said:
 
  I am assuming that if the laptop has sufficiently important data on  
  it to warrant the above steps then I am also clueful enough to:
 (A)  Not carry the laptop around unsecured areas,
 (B)  Keep a close enough eye on it and be aware that it's gone by  
  the time they get to step 2, OR
 (C)  Pay somebody to build me a better physical chassis for my laptop
 
 Building a better chassis can be a challenge when the threat model really
 *does* include attacks by a well-funded TLA.
 
 http://www.epic.org/crypto/scarfo/murch_aff.pdf

Thanks for the link! And this was ... 1999 (!) Probably goes without
saying that physical hacking tricks have only become even more refined/
perfected by now :-)


 The FBI did an *initial* entry to survey the hardware, and then a total of
 *five* other entries before they actually installed it.  Note the technical
 and legal requirements required on the KLS (it had to, among other things,
 capture PGP passphrases but *not* anything that was typed online).


Thanks,

Satyam
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 5/4] [-mm patch] Rename macros returning the size.

[Satyam Sharma]

Hi,


On Thu, 20 Sep 2007, Ken'ichi Ohmichi wrote:
 
 [PATCH 5/4] [-mm patch] Rename macros returning the size.
  The #define SIZE() should be renamed STRUCT_SIZE() since it's always 
  returning the size of the struct with a given name.  This would allow 
  TYPEDEF_SIZE() to simply become SIZE() since it need not be used 
  exclusively for typedefs. This idea is David Rientjes's.
  http://www.ussg.iu.edu/hypermail/linux/kernel/0709.1/1964.html
 
 Thanks
 Ken'ichi Ohmichi
 
 ---
 Signed-off-by: David Rientjes [EMAIL PROTECTED]

Hmm, I think adding a s-o-b line for David here isn't quite correct.
When someone reviews a patch and gives a suggestion, you only need to
copy him on the next iteration (and he may ack it or whatever, if he
wants) -- but adding a s-o-b line like that ends up (incorrectly)
denoting that he came between the author-to-git-commit chain ...


 Signed-off-by: Ken'ichi Ohmichi [EMAIL PROTECTED]

 --- a/include/linux/kexec.h   2007-09-18 15:22:19.0 +0900
 +++ b/include/linux/kexec.h   2007-09-18 15:23:22.0 +0900
 @@ -131,10 +131,10 @@ unsigned long paddr_vmcoreinfo_note(void
   vmcoreinfo_append_str(SYMBOL(%s)=%lx\n, #name, (unsigned long)name)
  #define VMCOREINFO_SIZE(name) \
   vmcoreinfo_append_str(SIZE(%s)=%lu\n, #name, \
 -   (unsigned long)sizeof(struct name))
 -#define VMCOREINFO_TYPEDEF_SIZE(name) \
 - vmcoreinfo_append_str(SIZE(%s)=%lu\n, #name, \
 (unsigned long)sizeof(name))
 +#define VMCOREINFO_STRUCT_SIZE(name) \
 + vmcoreinfo_append_str(SIZE(%s)=%lu\n, #name, \
 +   (unsigned long)sizeof(struct name))
  #define VMCOREINFO_OFFSET(name, field) \
   vmcoreinfo_append_str(OFFSET(%s.%s)=%lu\n, #name, #field, \
 (unsigned long)(((struct name *)0)-field))

Please use %zu and lose all the ugly (unsigned long) casts here.

Otherwise it looks good,

Satyam
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/4] [-mm patch] use the existing offsetof().

[Satyam Sharma]

On Thu, 20 Sep 2007, Ken'ichi Ohmichi wrote:
 
 [PATCH 6/4] [-mm patch] use the existing offsetof().
  It is better that offsetof() is used for VMCOREINFO_OFFSET().
  This idea is Joe Perches's.
 
 
 
 Thanks
 Ken'ichi Ohmichi 
 
 ---
 Signed-off-by: Joe Perches [EMAIL PROTECTED]

Same deal here ...


 Signed-off-by: Ken'ichi Ohmichi [EMAIL PROTECTED]
 ---
 diff -rpuN a/include/linux/kexec.h b/include/linux/kexec.h
 --- a/include/linux/kexec.h   2007-09-18 15:23:22.0 +0900
 +++ b/include/linux/kexec.h   2007-09-18 15:27:29.0 +0900
 @@ -137,7 +137,7 @@ unsigned long paddr_vmcoreinfo_note(void
 (unsigned long)sizeof(struct name))
  #define VMCOREINFO_OFFSET(name, field) \
   vmcoreinfo_append_str(OFFSET(%s.%s)=%lu\n, #name, #field, \
 -   (unsigned long)(((struct name *)0)-field))
 +   (unsigned long)offsetof(struct name, field))
  #define VMCOREINFO_LENGTH(name, value) \
   vmcoreinfo_append_str(LENGTH(%s)=%lu\n, #name, (unsigned long)value)

... and here. Use %zu and lose the casts.

BTW I don't think these macros are such a big win in readability or
code clarity anyway. And I noticed that there are still some open
calls to vmcoreinfo_append_str() in kexec.c (such as for the OS_RELEASE
case) which you haven't macro-ized ... we end up having code that looks
like:

vmcoreinfo_append_str(...);
...

...
VMCOREINFO_OFFSET(...);
...

and it's not even obvious (unless you follow on to the definition of the
later macro) that the above two are *exactly* the same. So you could also
consider just losing the macros altogether.


Satyam
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [UFS] fs/ufs/super.c misreads the file system state

[Satyam Sharma]

On Wed, 19 Sep 2007, Leonid Kalev wrote:
 
 ufs_get_fs_state() needs the file system type to read the state from the
 correct place in the superblock. It takes the type from UFS_SB(sb)-s_flags,
 but that value is stored after the first call to ufs_get_fs_state(). The patch
 below moves the assignment of s_flags up, before the first call to
 ufs_get_fs_state().
 
 The patch is against linux-2.6.23-rc6-git7, but it applies (with offset) to
 2.6.22 and to earlier versions as well. It has been tested on the Solaris
 flavor of UFS (ufstype=sunx86) - with this change, the file system can be used
 in read-write mode.

The same fix is already in 2.6.23-rc6-mm1 (don't know if it's lined up
for .23 though, I think it should be).


Satyam

PS: All my mails to Evgeniy Dushistov never reach him. The mail.ru server
is absolutely *evil* and simply rejects all mails unless the sender
is already whitelisted by the reciever (!)
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

[Satyam Sharma]

Hi Maciej,


On Thu, 20 Sep 2007, Maciej W. Rozycki wrote:
 
 On Wed, 19 Sep 2007, Andrew Morton wrote:
 
   patch-mips-2.6.23-rc5-20070904-pmag-ba-err-2
   diff -up --recursive --new-file 
   linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c 
   linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c
   --- linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c   
   2007-02-21 05:56:47.0 +
   +++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c 
   2007-09-18 10:56:51.0 +
   @@ -147,16 +147,23 @@ static int __init pmagbafb_probe(struct 
 resource_size_t start, len;
 struct fb_info *info;
 struct pmagbafb_par *par;
   + int err = 0;
  
  This initialisation to zero is not good.
  
  Because if some error-path code forgot to do `err = -EFOO' then probe()
  will return zero and the driver will leave things in half-initialised state
  and will then proceed as if things had succeeded.  It will crash.
 
  GCC used to complain: `foo' might be used uninitialized... and this is 
 the usual cure; let me see if this not the case anymore (I have 4.1.2).

Even so, initializing to zero isn't quite good. You could use the
uninitialized_var() (once you've confirmed that the warning is bogus).
However, some maintainers may still nack uninitialized_var() usage,
quite legitimately.


  So it's better to leave this local uninitialised, because we really want to
  get that compiler warning if someone forgot to set the return value.
 
  Yes of course, barring the issue mentioned.  Note the message above is 
 not the same as: `foo' is used uninitialized... that would be reported 
 in the case which you are concerned of.

Firstly, may be used uninitialized can still be a bug.

Secondly, latest gcc is *horribly* buggy (and has been so for last several
releases including 4.1, 4.2 and 4.3 -- 3.x was good). See:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33327
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501

We'd been hurling all sorts of abuses on gcc for quite long (when it fails
to detect these false positive cases), but now, it turns out it is quite
easy to write *genuinely* buggy code that still won't get any warnings,
neither the is used nor may be used one!

In short, there are three ways to fix these false positive warnings:

1. Do nothing, there are enough uninitialized variable warnings anyway,
   and hopefully, one day GCC would clean up its act.

2. Use uninitialized_var() to shut it up (only if it's genuinely bogus).

3. Do something like the following legendary patch [1]:

http://kegel.com/crosstool/crosstool-0.43/patches/linux-2.6.11.3/arch_alpha_kernel_srcons.patch

i.e., explicitly change the structure/logic of the function to make it
obvious enough to gcc that the variable will not be used uninitialized.


Satyam

[1] That was a funny case -- the alpha linux maintainer is also a gcc
maintainer. Alpha even sets -Werror, so either he had to fix the
kernel code that produced the warning, or go fix GCC to not warn
about it -- he chose the former :-)
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: error from checkpatch.pl version 0.10

[Satyam Sharma]

On Thu, 20 Sep 2007, Tetsuo Handa wrote:
 
 I checked my patch using checkpatch.pl version 0.10
 and I got the following error.
 
   ERROR: need consistent spacing around '*' (ctx:WxV)
   #2334: FILE: security/tomoyo/common.c:2306:
   +static unsigned int tmy_poll(struct file *file, poll_table *wait)
   ^

Looks like a checkpatch.pl bug to me -- that was nothing to warn about.


 What action should I take?
 Ignore this error because poll_table is used everywhere?
 Replace poll_table with struct poll_table_struct according to
 definition of poll_table?

Yeah, this would be better to do anyway (and rename poll_table_struct
to just poll_table).

   typedef struct poll_table_struct {
   poll_queue_proc qproc;
   } poll_table;

So:

struct poll_table {
poll_queue_proc qproc;
};

In general the kernel's codingstyle consensus is to avoid adding typedefs.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

2.6.23-rc6-mm1: Build failures on ppc64_defconfig

[Satyam Sharma]

On Thu, 20 Sep 2007, Mel Gorman wrote:
 
 allmodconfig on ppc64 fails to build with the following error

BTW ppc64_defconfig didn't quite like 2.6.23-rc6-mm1 either ...
IIRC I got build failures in:

drivers/ata/pata_scc.c
drivers/md/raid6int8.c
drivers/net/spider_net.c
drivers/net/pasemi_mac.c
drivers/pci/hotplug/rpadlpar_sysfs.c

I was in a hurry so didn't record the errors, just noted down the files
and disabled them from .config and continued building ... I'll get back
to fixing these up tonight (if you don't do that by then already).


Satyam
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

[Satyam Sharma]

On Thu, 20 Sep 2007, Markus Gothe wrote:
 
 GCC 4.1.2 has been stable for a long time now, maybe you better
 upgrade your binutils instead...

I'd been using 4.2.1 -- I don't want to downgrade to 4.1.2. (btw from
the discussion on gcc's bugzilla it appears the bug wasn't resolved
in 4.1.2 either?)

Satyam

 Satyam Sharma wrote:
  Hi Maciej,
 
 
  On Thu, 20 Sep 2007, Maciej W. Rozycki wrote:
   
   On Wed, 19 Sep 2007, Andrew Morton wrote:

This initialisation to zero is not good.

Because if some error-path code forgot to do `err = -EFOO' then
probe() will return zero and the driver will leave things in
half-initialised state and will then proceed as if things had
succeeded.  It will crash.
   
   GCC used to complain: `foo' might be used uninitialized... and
   this is the usual cure; let me see if this not the case anymore
   (I have 4.1.2).
 
  Even so, initializing to zero isn't quite good. You could use the
  uninitialized_var() (once you've confirmed that the warning is
  bogus). However, some maintainers may still nack
  uninitialized_var() usage, quite legitimately.
 
 
So it's better to leave this local uninitialised, because we
really want to get that compiler warning if someone forgot to
set the return value.
   Yes of course, barring the issue mentioned.  Note the message
   above is not the same as: `foo' is used uninitialized... that
   would be reported in the case which you are concerned of.
 
  Firstly, may be used uninitialized can still be a bug.
 
  Secondly, latest gcc is *horribly* buggy (and has been so for last
  several releases including 4.1, 4.2 and 4.3 -- 3.x was good). See:
 
  http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33327
  http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501
 
  We'd been hurling all sorts of abuses on gcc for quite long (when
  it fails to detect these false positive cases), but now, it turns
  out it is quite easy to write *genuinely* buggy code that still
  won't get any warnings, neither the is used nor may be used
  one!
 
  In short, there are three ways to fix these false positive
  warnings:
 
  1. Do nothing, there are enough uninitialized variable warnings
  anyway, and hopefully, one day GCC would clean up its act.
 
  2. Use uninitialized_var() to shut it up (only if it's genuinely
  bogus).
 
  3. Do something like the following legendary patch [1]:
 
  http://kegel.com/crosstool/crosstool-0.43/patches/linux-2.6.11.3/arch_alpha_kernel_srcons.patch
 
 
  i.e., explicitly change the structure/logic of the function to make
  it obvious enough to gcc that the variable will not be used
  uninitialized.
 
 
  Satyam
 
  [1] That was a funny case -- the alpha linux maintainer is also a
  gcc maintainer. Alpha even sets -Werror, so either he had to fix
  the kernel code that produced the warning, or go fix GCC to not
  warn about it -- he chose the former :-)
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

[Satyam Sharma]

On Thu, 20 Sep 2007, Maciej W. Rozycki wrote:
 
  Perhaps preinitialising to an error value such as -EINVAL would be of
 more sense.  This way any error paths lacking initialisation are still
 reported as errors, even though the classification might be wrong.

Eeee ... at least I wouldn't prefer that. Why not simply use the
int x = x; trick (which is what uninitialized_var() does) -- it shuts
up the warning, and does *nothing* else. The bug will not be hidden, if
there's bad misbehaviour happening due to the bug, it will continue to
happen that way -- thus bringing our attention to it. Pre-initializing
to -EINVAL (or whatever) has the problem that when the bug actually
triggers, something unrelated might happen higher up the callchain, and
we'd be scratching our heads in a why are we getting a -EINVAL here?
kind of way ... worse still, we might think that this was _really_ an
EINVAL and go about debugging it ...

Plus, pre-initializing to -EINVAL (or even 0) will waste some bytes in
kernel text size, but no such overhead with uninitialized_var() :-)


Satyam
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] Time to make CONFIG_PARAVIRT non-experimental.

[Satyam Sharma]

On Tue, 18 Sep 2007, Charles N Wyble wrote:
 
 Andi Kleen wrote:
  
  Besides it's bad taste and taste is very important.
 
 Well it's bad taste for you (one person).

FWIW, my opinion is the same as Andi's here. Please, let's avoid this
disease -- unless *absolutely* required, stuff shouldn't be default y.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] Time to make CONFIG_PARAVIRT non-experimental.

[Satyam Sharma]

On Thu, 20 Sep 2007, Charles N Wyble wrote:
 
 Satyam Sharma wrote:
  
  On Tue, 18 Sep 2007, Charles N Wyble wrote:
  Andi Kleen wrote:
  Besides it's bad taste and taste is very important.
  Well it's bad taste for you (one person).
  
  FWIW, my opinion is the same as Andi's here. Please, let's avoid this
  disease -- unless *absolutely* required, stuff shouldn't be default y.
 
 I am curious why you think it shouldn't be default why? Bad taste? Can
 you provide data about how it will harm things?

Clear CONFIG_PARAVIRT from your .config and try make oldconfig, and find
out for yourself :-)

You'll end up having to answer questions for all other config options
below it, that you never cared about, that you didn't know even existed.
You'd obviously (I would, at least) feel annoyed by it all ... this is
almost a regression :-)

And for those who automate the oldconfig process using yes, well, this
default y *will* end up introducing a kernel text size regression --
all those config options they never had earlier, suddenly got enabled.

Besides, like Andi said, selecting or enabling CONFIG_PARAVIRT is quite
pointless in the first place -- it's just a menuconfig symbol, not a
real kconfig symbol that _actually_ controls code in the Makefiles.


Satyam
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] unify DMA_..BIT_MASK definitions: v3.1

[Satyam Sharma]

On Tue, 18 Sep 2007, Borislav Petkov wrote:
> 
> These patches remove redundant DMA_..BIT_MASK definitions across two
> drivers. The computation of the majority of the bitmasks is done by the
> compiler. The initial split of the patch touching each a different file got
> removed due to possible git bisect breakage.
> 
> Andrew, can you please apply this patch for it touches drivers maintained by
> different people and i there might be responsibility issues, imho.
> 
> Signed-off-by: Borislav Petkov <[EMAIL PROTECTED]>
> Cc: Jeremy Fitzhardinge <[EMAIL PROTECTED]>
> Cc: Muli Ben-Yehuda <[EMAIL PROTECTED]>

Thanks, Borislav.

Reviewed-by: Satyam Sharma <[EMAIL PROTECTED]>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NFS4 authentification / fsuid

[Satyam Sharma]

On Wed, 19 Sep 2007, Kyle Moffett wrote:
> 
> > [all sorts of crap about spies in washington needing stronger protection
> > than your average consumer]
> 
> [snip]
> 
> [...] all the bullcrap about foreign intelligence

Hehe, again, *you* started all the "bullcrap" about foreign "governments"
in the first place :-)


> is just drawing
> focus off of how easy it is to achieve *adequate* physical protection where it
  
> matters.

Ah, so you're qualifying the discussion with the nice and subjective
"adequate" ... (you're still wrong, of course)


> Of course, this also relies on being able to teach the stupid lusers with the
> laptops not to give their boot password to the "service tech on the phone"

Let's stick on-topic here ... remember "securing a system against attacker
with physical access is fairly simple" ?

[ Took the liberty of removing some irrelevant digressions -- didn't see
  any solid security scheme that fulfils/justifies your earlier claim over
  there. ]


> > > If your system equates end-user with attacker
> > 
> > "If"? Was there ever any doubt?
> > 
> > Heh, did you even read the thread you just replied to?
> 
> Yes I did [...]

No, you didn't -- it was obvious from your reply :-)

> and I wanted to make it *really* clear that with average hardware
> you can properly protect against virtually all of the *common* attack vectors.
 ^^

But what gave you the impression we're interested in discussing "common"
or "adequate enough" attack vectors here?

See, if you have something useful/new to contribute to the discussion,
that we don't already know, then please don't hold back and feel free to
do so ...


Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] powerpc: Avoid pointless WARN_ON(irqs_disabled()) from panic codepath

[Satyam Sharma]

> On Mon, 17 Sep 2007 18:37:49 -0700
> Randy Dunlap <[EMAIL PROTECTED]> wrote:
> 
> > On Tue, 18 Sep 2007 05:13:40 +0530 (IST) Satyam Sharma wrote:
> > 
> > > Untested (not even compile-tested) patch.
> > > Could someone point me to ppc32/64 cross-compilers for i386?
> > 
> > OSDL had some, but those are gone now.
> > I downloaded all of them and still use them, although it would
> > be good to have some more recent versions of them.
> > 
> > I put the power* compiler tarballs here:
> > 
> > http://userweb.kernel.org/~rdunlap/cross-compilers/

Thanks -- BTW I made some simple changes to the tree structure in there
and added a few distcc [*] related scriptlets. The resulting tar ball:

http://www.cse.iitk.ac.in/users/ssatyam/powerpc64-unknown-linux-gnu.tar.bz2

can be made to work with Andrew's nice "xb" script with the following
trivial patch:


--- cross-compilers/read-me.txt~powerpc64   2007-09-19 14:39:01.0 
+0530
+++ cross-compilers/read-me.txt 2007-09-19 14:44:29.0 +0530
@@ -3,7 +3,7 @@ i386 cross-compilation binaries for seve
 on RH FC5 and RH FC6 i386 and x86_64.
 
 - untar the tarball in /
-- setenv ARCH sparc64 (or alpha, arm, i386, ia64, m68k, mips, s390, sparc)
+- setenv ARCH sparc64 (or alpha, arm, i386, ia64, m68k, mips, powerpc64, s390, 
sh4, sparc, x86_64)
 - xb mrproper
 - xb allmodconfig
 - xb
--- cross-compilers/xb~powerpc642007-09-19 14:40:09.0 +0530
+++ cross-compilers/xb  2007-09-19 14:52:46.0 +0530
@@ -31,6 +31,7 @@ I=vmlinux
 [ $ARCH = m68k ] &&CT=gcc-4.1.0-glibc-2.3.6
 [ $ARCH = mips ] &&CT=gcc-3.4.5-glibc-2.3.6
 [ $ARCH = powerpc ] && CT=gcc-4.1.0-glibc-2.3.6 && XARCH=powerpc-405-linux-gnu
+[ $ARCH = powerpc64 ] &&   CT=gcc-3.4.0-glibc-2.3.2 && export ARCH=powerpc
 [ $ARCH = s390 ] &&CT=gcc-4.1.0-glibc-2.3.6
 [ $ARCH = sh ] &&  CT=gcc-3.4.5-glibc-2.3.6 && XARCH=sh4-unknown-linux-gnu
 [ $ARCH = sparc ] &&   CT=gcc-4.1.0-glibc-2.3.6


On Mon, 17 Sep 2007, Josh Boyer wrote:
> 
> Crosstool is widely used.  It'll build several combinations of
> gcc/binutils/glibc for you.  
> 
> http://www.kegel.com/crosstool/

In fact, it turns out OSDL's cross-compiler toolchains were built with
crosstool itself. Should also add that those OSDL compilers are too old
(gcc version 3.4.x-3.5.x mostly -- my build was totally spammed with those
"+m" in asm constraints related warnings), so I'll try and build a few
more recent ones (at least for the more popular platforms) over the
weekend too.


Satyam


[*] But I'm a bit skeptical if the distcc stuff in "xb" works as intended.
Has anybody used that successfully? Will test it over the weekend ...
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NFS4 authentification / fsuid

[Satyam Sharma]

On Wed, 19 Sep 2007, Kyle Moffett wrote:
> 
> On Sep 18, 2007, at 19:44:59, Satyam Sharma wrote:
> > 
> > The whole *point* here is to secure against physical access -- then how can
^^^
> > you assume "barring disassembling the system"? If you're not considering
> > attacks such as those, then how _are_ you solving the physical access
> > problem in the first place? :-)
> 
> [snip lots of totally irrelevant stuff]

???

What is your point, really?

Let me repeat the situation:

I own a computer (maybe portable one, such as a laptop) that I want to
protect from attackers with physical access to my system. You're proposing
a scheme that claims to secure it (against attackers with physical access)
but assumes: "barring disassembling the system".

Dude, looks like you're selling snake oil here.


> > > Under this setup, tinkering with my BIOS does virtually nothing; the only
> > > avenues of attack are strictly of the "Install a hardware keylogger"
> > > variety.
> > 
> > Doesn't flashing/replacing your BIOS firmware/chip count as tinkering?  Then
> > I don't really need a "hardware keylogger", do I ...
> 
> Ok, so you are saying your plan of attack on this system would be:
>  1)  Steal the laptop such that I don't notice it has been stolen
>  2)  Open it up
>  3)  Replace the very-vendor-specific BIOS chip with a reflashed one with
> sufficient storage to do all the things the old BIOS could *AND* have enough
> storage for an entire replacement kernel binary with a built-in keylogger, as
> well as some storage for the logged password
>  4)  Return the laptop, again such that I don't notice it has been missing
>  5)  Wait for me to boot and type my password
>  6)  Somehow recover the laptop *yet* *again* to get the password back off of
> it and decrypt the disk

Precisely. Do you think the above attack is "fantastical"?

Wow, you're amazingly naive ... good luck ;-)

[ See, if it's only your kid sister that you want to "protect" your
  36GB worth of porn from, then you might as well use Windoze and one of
  those cute little "folder-locking" tool that we wrote back in 5th grade.

  However, if "hapless North Korean spy in Washington" describes you more
  accurately, then you better be ready for all sorts of attacks -- from
  exploding cigars [1], to poisoned ballpoint pens [2] :-)

  In short, you have no clue what you're talking about, and thankfully
  I'm not using any security software you had any part in designing :-) ]


> Yes it "can be done", but so can dumping the firmware for an iPod out through
> the built-in piezo clicker[1].  USE SOME COMMON SENSE HERE PEOPLE!!!  The only
> "unbreakable" computer is one always disconnected and off under armed guard in
> a bank vault, and even then it's only as secure as the bank in which it is
> stored (which get broken into on occasion).

Thanks for repeatedly making *my* point :-)

_You_ are the one who claimed protecting systems from attackers with
physical access to be a "fairly simple" problem ... and here you're
mentioning how *difficult* it is ...


> I am assuming that if the laptop has sufficiently important data on it to
> warrant the above steps then I am also clueful enough to:
>  (A)  Not carry the laptop around unsecured areas,

You might carry it home, might you not? What if your lover/girlfriend/wife
is one of them? [3]


>  (B)  Keep a close enough eye on it and be aware that it's gone by the time
> they get to step 2, OR

Hmm, you'd need to be a mutant to keep "close enough eyes" on your stuff
while you're sleeping ... or drugged (?)


>  (C)  Pay somebody to build me a better physical chassis for my laptop

ROTFL ... these "workarounds" above are even more hilarious than your
earlier "fairly simple" claim.


> We are talking about *STANDARD* laptop systems with reasonably alert users.
> If the user doesn't know how to properly protect the stuff on the laptop then
> they probably don't know how to properly protect the other copy in their
> heads, either.

Dude, if the data in there is really that important, then better not
store it on a computer / disk at all :-)


> Besides, if some government wanted the data on your laptop
> that bad they'd just pick you up in the middle of the night and torture your
> password out of you.

Surprisingly, you have (somewhat of) a point (!)


> On Sep 18, 2007, at 19:48:16, Satyam Sharma wrote:
> > On Fri, 7 Sep 2007, Kyle Moffett wrote:
> > > So you can't draw any relationships between "Protect the end-user" with
> > > "Protect the device FROM the end-user", the former can be done very
>

Re: NFS4 authentification / fsuid

[Satyam Sharma]

On Wed, 19 Sep 2007, Kyle Moffett wrote:
 
 On Sep 18, 2007, at 19:44:59, Satyam Sharma wrote:
  
  The whole *point* here is to secure against physical access -- then how can
^^^
  you assume barring disassembling the system? If you're not considering
  attacks such as those, then how _are_ you solving the physical access
  problem in the first place? :-)
 
 [snip lots of totally irrelevant stuff]

???

What is your point, really?

Let me repeat the situation:

I own a computer (maybe portable one, such as a laptop) that I want to
protect from attackers with physical access to my system. You're proposing
a scheme that claims to secure it (against attackers with physical access)
but assumes: barring disassembling the system.

Dude, looks like you're selling snake oil here.


   Under this setup, tinkering with my BIOS does virtually nothing; the only
   avenues of attack are strictly of the Install a hardware keylogger
   variety.
  
  Doesn't flashing/replacing your BIOS firmware/chip count as tinkering?  Then
  I don't really need a hardware keylogger, do I ...
 
 Ok, so you are saying your plan of attack on this system would be:
  1)  Steal the laptop such that I don't notice it has been stolen
  2)  Open it up
  3)  Replace the very-vendor-specific BIOS chip with a reflashed one with
 sufficient storage to do all the things the old BIOS could *AND* have enough
 storage for an entire replacement kernel binary with a built-in keylogger, as
 well as some storage for the logged password
  4)  Return the laptop, again such that I don't notice it has been missing
  5)  Wait for me to boot and type my password
  6)  Somehow recover the laptop *yet* *again* to get the password back off of
 it and decrypt the disk

Precisely. Do you think the above attack is fantastical?

Wow, you're amazingly naive ... good luck ;-)

[ See, if it's only your kid sister that you want to protect your
  36GB worth of porn from, then you might as well use Windoze and one of
  those cute little folder-locking tool that we wrote back in 5th grade.

  However, if hapless North Korean spy in Washington describes you more
  accurately, then you better be ready for all sorts of attacks -- from
  exploding cigars [1], to poisoned ballpoint pens [2] :-)

  In short, you have no clue what you're talking about, and thankfully
  I'm not using any security software you had any part in designing :-) ]


 Yes it can be done, but so can dumping the firmware for an iPod out through
 the built-in piezo clicker[1].  USE SOME COMMON SENSE HERE PEOPLE!!!  The only
 unbreakable computer is one always disconnected and off under armed guard in
 a bank vault, and even then it's only as secure as the bank in which it is
 stored (which get broken into on occasion).

Thanks for repeatedly making *my* point :-)

_You_ are the one who claimed protecting systems from attackers with
physical access to be a fairly simple problem ... and here you're
mentioning how *difficult* it is ...


 I am assuming that if the laptop has sufficiently important data on it to
 warrant the above steps then I am also clueful enough to:
  (A)  Not carry the laptop around unsecured areas,

You might carry it home, might you not? What if your lover/girlfriend/wife
is one of them? [3]


  (B)  Keep a close enough eye on it and be aware that it's gone by the time
 they get to step 2, OR

Hmm, you'd need to be a mutant to keep close enough eyes on your stuff
while you're sleeping ... or drugged (?)


  (C)  Pay somebody to build me a better physical chassis for my laptop

ROTFL ... these workarounds above are even more hilarious than your
earlier fairly simple claim.


 We are talking about *STANDARD* laptop systems with reasonably alert users.
 If the user doesn't know how to properly protect the stuff on the laptop then
 they probably don't know how to properly protect the other copy in their
 heads, either.

Dude, if the data in there is really that important, then better not
store it on a computer / disk at all :-)


 Besides, if some government wanted the data on your laptop
 that bad they'd just pick you up in the middle of the night and torture your
 password out of you.

Surprisingly, you have (somewhat of) a point (!)


 On Sep 18, 2007, at 19:48:16, Satyam Sharma wrote:
  On Fri, 7 Sep 2007, Kyle Moffett wrote:
   So you can't draw any relationships between Protect the end-user with
   Protect the device FROM the end-user, the former can be done very
   reliably to whatever level of risk-reduction you need and the latter can't
   practically be done at all.
  
  Well, you're the one who called solving the physical access problem easy
  here ... :-)
 
 If your system equates end-user with attacker
  ^^

If? Was there ever any doubt?

Heh, did you even read the thread you just replied to?

We're talking of consoles / hardware sold by commercial companies to
users here, where they want explicitly want to prevent

Re: [PATCH] powerpc: Avoid pointless WARN_ON(irqs_disabled()) from panic codepath

[Satyam Sharma]

 On Mon, 17 Sep 2007 18:37:49 -0700
 Randy Dunlap [EMAIL PROTECTED] wrote:
 
  On Tue, 18 Sep 2007 05:13:40 +0530 (IST) Satyam Sharma wrote:
  
   Untested (not even compile-tested) patch.
   Could someone point me to ppc32/64 cross-compilers for i386?
  
  OSDL had some, but those are gone now.
  I downloaded all of them and still use them, although it would
  be good to have some more recent versions of them.
  
  I put the power* compiler tarballs here:
  
  http://userweb.kernel.org/~rdunlap/cross-compilers/

Thanks -- BTW I made some simple changes to the tree structure in there
and added a few distcc [*] related scriptlets. The resulting tar ball:

http://www.cse.iitk.ac.in/users/ssatyam/powerpc64-unknown-linux-gnu.tar.bz2

can be made to work with Andrew's nice xb script with the following
trivial patch:


--- cross-compilers/read-me.txt~powerpc64   2007-09-19 14:39:01.0 
+0530
+++ cross-compilers/read-me.txt 2007-09-19 14:44:29.0 +0530
@@ -3,7 +3,7 @@ i386 cross-compilation binaries for seve
 on RH FC5 and RH FC6 i386 and x86_64.
 
 - untar the tarball in /
-- setenv ARCH sparc64 (or alpha, arm, i386, ia64, m68k, mips, s390, sparc)
+- setenv ARCH sparc64 (or alpha, arm, i386, ia64, m68k, mips, powerpc64, s390, 
sh4, sparc, x86_64)
 - xb mrproper
 - xb allmodconfig
 - xb
--- cross-compilers/xb~powerpc642007-09-19 14:40:09.0 +0530
+++ cross-compilers/xb  2007-09-19 14:52:46.0 +0530
@@ -31,6 +31,7 @@ I=vmlinux
 [ $ARCH = m68k ] CT=gcc-4.1.0-glibc-2.3.6
 [ $ARCH = mips ] CT=gcc-3.4.5-glibc-2.3.6
 [ $ARCH = powerpc ]  CT=gcc-4.1.0-glibc-2.3.6  XARCH=powerpc-405-linux-gnu
+[ $ARCH = powerpc64 ]CT=gcc-3.4.0-glibc-2.3.2  export ARCH=powerpc
 [ $ARCH = s390 ] CT=gcc-4.1.0-glibc-2.3.6
 [ $ARCH = sh ]   CT=gcc-3.4.5-glibc-2.3.6  XARCH=sh4-unknown-linux-gnu
 [ $ARCH = sparc ]CT=gcc-4.1.0-glibc-2.3.6


On Mon, 17 Sep 2007, Josh Boyer wrote:
 
 Crosstool is widely used.  It'll build several combinations of
 gcc/binutils/glibc for you.  
 
 http://www.kegel.com/crosstool/

In fact, it turns out OSDL's cross-compiler toolchains were built with
crosstool itself. Should also add that those OSDL compilers are too old
(gcc version 3.4.x-3.5.x mostly -- my build was totally spammed with those
+m in asm constraints related warnings), so I'll try and build a few
more recent ones (at least for the more popular platforms) over the
weekend too.


Satyam


[*] But I'm a bit skeptical if the distcc stuff in xb works as intended.
Has anybody used that successfully? Will test it over the weekend ...
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NFS4 authentification / fsuid

[Satyam Sharma]

On Wed, 19 Sep 2007, Kyle Moffett wrote:
 
  [all sorts of crap about spies in washington needing stronger protection
  than your average consumer]
 
 [snip]
 
 [...] all the bullcrap about foreign intelligence

Hehe, again, *you* started all the bullcrap about foreign governments
in the first place :-)


 is just drawing
 focus off of how easy it is to achieve *adequate* physical protection where it
  
 matters.

Ah, so you're qualifying the discussion with the nice and subjective
adequate ... (you're still wrong, of course)


 Of course, this also relies on being able to teach the stupid lusers with the
 laptops not to give their boot password to the service tech on the phone

Let's stick on-topic here ... remember securing a system against attacker
with physical access is fairly simple ?

[ Took the liberty of removing some irrelevant digressions -- didn't see
  any solid security scheme that fulfils/justifies your earlier claim over
  there. ]


   If your system equates end-user with attacker
  
  If? Was there ever any doubt?
  
  Heh, did you even read the thread you just replied to?
 
 Yes I did [...]

No, you didn't -- it was obvious from your reply :-)

 and I wanted to make it *really* clear that with average hardware
 you can properly protect against virtually all of the *common* attack vectors.
 ^^

But what gave you the impression we're interested in discussing common
or adequate enough attack vectors here?

See, if you have something useful/new to contribute to the discussion,
that we don't already know, then please don't hold back and feel free to
do so ...


Satyam
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] unify DMA_..BIT_MASK definitions: v3.1

[Satyam Sharma]

On Tue, 18 Sep 2007, Borislav Petkov wrote:
 
 These patches remove redundant DMA_..BIT_MASK definitions across two
 drivers. The computation of the majority of the bitmasks is done by the
 compiler. The initial split of the patch touching each a different file got
 removed due to possible git bisect breakage.
 
 Andrew, can you please apply this patch for it touches drivers maintained by
 different people and i there might be responsibility issues, imho.
 
 Signed-off-by: Borislav Petkov [EMAIL PROTECTED]
 Cc: Jeremy Fitzhardinge [EMAIL PROTECTED]
 Cc: Muli Ben-Yehuda [EMAIL PROTECTED]

Thanks, Borislav.

Reviewed-by: Satyam Sharma [EMAIL PROTECTED]
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: iso9660 vs udf

[Satyam Sharma]

Hi Andries,


On Wed, 19 Sep 2007, Andries E. Brouwer wrote:
> 
> On Wed, Sep 19, 2007 at 05:48:28AM +0530, Satyam Sharma wrote:
> 
> > > > On the other hand, this filesystem announces itself as UDF
> > > > ("CD-RTOS" "CD-BRIDGE" "CDUDF File System - Adaptec Inc"),
> > > > perhaps the kernel code should be more robust.
> > 
> > Could you send the complete dmesg log, and what you mean with filesystem/
> > kernel (incorrectly?) announcing it as UDF here ... I agree with Jan,
> > this sounds like an issue with mount(8) to me.
> 
> You already got the relevant part of the dmesg log. Slightly more below.

> Failed mount:
> UDF-fs INFO UDF 0.9.8.1 (2004/29/09) Mounting volume 'Wisk1956-82', timestamp 
> 2006/03/07 16:26 (1078)
> udf: udf_read_inode(ino 547) failed !bh
> UDF-fs: Error in udf_iget, block=1, partition=1

Ok, like said, this comes from udf_fill_super(), but which shouldn't
have been called for this CD in the first place -- i.e. mount(8) shouldn't
have tried to mount a non-UDF filesystem as UDF (unless explicitly asked
as such). I was actually asking for the logs explaining why you thought
the _kernel_ incorrectly "announced" it as an UDF filesystem.

Hmm ... those "CD-RTOS", "CD-BRIDGE" and "CDUDF File System - Adaptec Inc"
bits are not dmesg output, are they? Looks like "hwinfo --cdrom" or
"isoinfo" or some such.

> I think the filesystem can be treated both as iso9660 and as udf,
> at least that is what I seem to recall CD-BRIDGE means.  Thus,
> if the kernel cannot mount it as udf, I think it is a kernel flaw.
> Given that kernel flaw, and the fact that mounting as iso9660 works,
> mount(8) could work around the kernel problem by guessing iso9660.
> But maybe we should first try to fix the kernel.

I don't think that is what CD-BRIDGE means -- so no kernel flaw :-)
What happened here is simply that in the absence of a "-t" option,
mount(8) defaulted (probably due to incorrect heuristics?) to UDF for
some reason, thereby obviously failing. I don't know who maintains
mount(8) / util-linux package, or do distributions have their own
maintainers these days (?)


Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Reduce __print_symbol/sprint_symbol stack usage.

[Satyam Sharma]

Hi Gilboa,


On Sat, 15 Sep 2007, Gilboa Davara wrote:
> 
> This is my second stab at solving the "stack over flow due to
> dump_strace when close to stack-overflow is detected by do_IRQ" problem.
> (Hopefully) this patch is creates less noise then the previous one.
> 
> [snip]
> > I'll try and create an option 2 (static allocation, minimal locking)
> > patch and post ASAP.
> > Hopefully it'll fare better. (While keeping the current interface intact
> > and reducing the damage/noise)
> 
> - Gilboa
> 
> --- linux-2.6/kernel/kallsyms.orig2007-09-15 11:46:54.0 +0300
> +++ linux-2.6/kernel/kallsyms.c   2007-09-15 21:06:55.0 +0300
> @@ -306,13 +306,14 @@ int lookup_symbol_attrs(unsigned long ad
>   return lookup_module_symbol_attrs(addr, size, offset, modname, name);
>  }
>  
> -/* Look up a kernel symbol and return it in a text buffer. */
> -int sprint_symbol(char *buffer, unsigned long address)
> +/* Internal version:
> +   Look up a kernel symbol and module name and return them to the
> + caller's buffer/namebuf buffers. */

/*
 * ...
 * ...
 */

is the general coding style here ...

> +int __sprint_symbol(char *buffer, char *namebuf, unsigned long address)
>  {
> - char *modname;
> - const char *name;
>   unsigned long offset, size;
> - char namebuf[KSYM_NAME_LEN];
> + const char *name;
> + char *modname;
>  
>   name = kallsyms_lookup(address, , , , namebuf);
>   if (!name)
> @@ -325,14 +326,35 @@ int sprint_symbol(char *buffer, unsigned
>   return sprintf(buffer, "%s+%#lx/%#lx", name, offset, size);
>  }
>  
> +/* Exported version:
> +   Look up a kernel symbol and return it in a text buffer. */

ditto.

> +int sprint_symbol(char *buffer, unsigned long address)
> +{
> + char namebuf[KSYM_NAME_LEN];

Hmm, don't we intend to push this array out of the stack too?

+   static char namebuf[KSYM_NAME_LEN];
+   static DEFINE_SPINLOCK(namebuf_lock);

here ?

> +
> + return __sprint_symbol(buffer, namebuf, address);

And you'd need to wrap spin_lock_irqsave()/spin_unlock_irqrestore()
around this call.

> +}


> +static DEFINE_SPINLOCK(symbol_lock);

Try to keep the declarations of a lock, and the data that it protects,
close together. Since this lock is being used to protect "buffer", it
makes sense to ...


>  /* Look up a kernel symbol and print it to the kernel messages. */
>  void __print_symbol(const char *fmt, unsigned long address)
>  {
> - char buffer[KSYM_SYMBOL_LEN];
> + /* Use static buffers instead of char array to reduce
> +  stack footprint in i386/4KSTACKS.
> +  Buffers must be protected against re-entry. */
> + static char namebuf[KSYM_NAME_LEN];
> + static char buffer[KSYM_SYMBOL_LEN];

... have it:

+   static DEFINE_SPINLOCK(buffer_lock);

here (note the name that exactly describes what the lock protects).

And the namebuf array isn't required here, it's already there in
sprint_symbol(), which you can call from ...

> + unsigned long flags;
> +
>  
> - sprint_symbol(buffer, address);
> + spin_lock_irqsave(_lock, flags);
> +
> + __sprint_symbol(buffer, namebuf, address);

here ... sprint_symbol() ?

>   printk(fmt, buffer);
> +
> + spin_unlock_irqrestore(_lock, flags);

But I still don't much like this :-(

More importantly, if a panic occurs *below* this callchain (and let's
say we ended up in this callchain because somebody put in a dump_stack()
somewhere for debugging purposes), then we'd have a deadlock on our hands,
and nothing gets printed for that panic.

I don't know who maintains this part of kernel code, but you can try
resubmitting (with the changes suggested above) to someone appropriate ...


Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: iso9660 vs udf

[Satyam Sharma]

Hi,

On Tue, 18 Sep 2007, Jan Kara wrote:
> 
> > Today I got a CD. MacOS does not mount it and Linux does not
> > mount it without an explicit filesystemtype option.
> > That is,
> > # mount /dev/hdc /dir -t iso9660
> > works fine, but
> > # mount /dev/hdc /dir
> > mount: you didn't specify a filesystem type for /dev/hdc
> >I will try type udf
> > mount: wrong fs type, bad option, bad superblock on /dev/hdc,
> >missing codepage or other error
> >In some cases useful info is found in syslog - try
> >dmesg | tail  or so
> > # dmesg | tail
> > UDF-fs INFO UDF 0.9.8.1 (2004/29/09) Mounting volume 'Wisk1956-82', 
> > timestamp 2006/03/07 16:26 (1078)
> > udf: udf_read_inode(ino 547) failed !bh
> > UDF-fs: Error in udf_iget, block=1, partition=1

That comes from udf_fill_super() but which shouldn't have been called
in the first place ...

> > Google gave me half a dozen other people that mentioned the same
> > problem (with the same inode 547). Clearly some CD mastering software
> > produces a format that Linux and MacOS do not handle easily.
> > 
> > One result of this letter will be that people with the same problem
> > learn via Google that using the "-t iso9660" option may help.
> > 
> > What goes wrong on the mount side is that when it hesitates between
> > iso9660 and udf it decides for udf when seeing "NSR02".
> > Maybe the heuristics in mount should be tuned.
>   Yes, this seems like a mount problem but you should contact mount
> maintainer for that... I guess hardly anyone will help you with this on
> this list.
> 
> > On the other hand, this filesystem announces itself as UDF
> > ("CD-RTOS" "CD-BRIDGE" "CDUDF File System - Adaptec Inc"),
> > perhaps the kernel code should be more robust.

Could you send the complete dmesg log, and what you mean with filesystem/
kernel (incorrectly?) announcing it as UDF here ... I agree with Jan,
this sounds like an issue with mount(8) to me.

> > If anybody feels responsible for mount and/or this kernel area
> > we might discuss.
>   I'm kind of taking care about UDF in kernel. What do you find
> inappropriate on the kernel reaction? You mean we should produce some
> better error message into the log?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NFS4 authentification / fsuid

[Satyam Sharma]

On Fri, 7 Sep 2007, Kyle Moffett wrote:
> 
> So you can't draw any relationships between "Protect the end-user" with
> "Protect the device FROM the end-user", the former can be done very reliably
   ^^^ *attacker*

> to whatever level of risk-reduction you need and the latter can't practically
> be done at all.

Well, you're the one who called solving the physical access problem
"easy" here ... :-)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NFS4 authentification / fsuid

[Satyam Sharma]

On Thu, 6 Sep 2007, Kyle Moffett wrote:
> 
> On Sep 06, 2007, at 19:35:14, Trond Myklebust wrote:
> > 
> > On Thu, 2007-09-06 at 19:30 -0400, Kyle Moffett wrote:
> > > 
> > > On Sep 06, 2007, at 11:06:16, J. Bruce Fields wrote:
> > > > The question of how to protect against someone with *physical*
   ^^^
> > > > access certainly is more difficult, but surely that's a separate
^^

> > > > problem.
> > > 
> > > Actually, that's a fairly simple problem (barring disassembling the system


> > > and attaching a hardware debugger).  You encrypt the root filesystem and
> > > require a password to boot (See: LUKS).  Debian has built-in support for
> > > installing onto fs-on-LVM-on-crypt-on-RAID, and it works quite well on all
> > > the laptops I use regularly.  It's not even much of a speed penalty; once
> > > you take the overhead of hitting a 5400RPM laptop drive you can chew
> > > thousands of cycles of CPU without anybody noticing (much).  Then all you
> > > have to do is burn a copy of your /boot with bootloader onto some
> > > read-only media (like a finalized CDROM/DVDROM) and you're set to go.
> > 
> > Disconnect battery, and watch boot password go 'poof!'.
> 
> Umm, I did say "encrypt the root filesystem", didn't I?  Booting my laptops
  ^^^

The whole *point* here is to secure against physical access -- then how
can you assume "barring disassembling the system"? If you're not
considering attacks such as those, then how _are_ you solving the
physical access problem in the first place? :-)


> this way follows this procedure:
>  1) Enter BIOS boot menu
>  2) Insert /boot CDROM
>  3) Select the "CDROM" entry
>  4) Wait for kernel to start and run through initramfs
>  5) Type password into the initramfs prompt so that it can DECRYPT THE ROOT
> FILESYSTEM
>  6) Continue to boot the system.
> 
> Under this setup, tinkering with my BIOS does virtually nothing; the only
> avenues of attack are strictly of the "Install a hardware keylogger" variety.

Doesn't flashing/replacing your BIOS firmware/chip count as tinkering?
Then I don't really need a "hardware keylogger", do I ...
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NFS4 authentification / fsuid

[Satyam Sharma]

On Fri, 7 Sep 2007, J. Bruce Fields wrote:
> 
> On Fri, Sep 07, 2007 at 01:32:52AM +0200, Trond Myklebust wrote:
> > Sorry. Of course, you have to copy the entire /lib, etc. onto the tmpfs,
> > but you get the gist
> > 
> > The point is that it is easy to subvert userspace if you have enough
> > privileges. In the above example it may not be entirely undetectable,
> > but who here is running a script on every login to check that / is
> > indeed uncompromised?
> 
> I suppose this is the motivation for things like the "secure attention
> key"?
> 
> But I'm most curious actually about to what degree the kernel itself is
> vulnerable to root (without a reboot).  Is disabling /dev/kmem and
> module-loading in theory enough?

No, not in theory, not in practice. But yeah, restricting an attacker's
ability to hack hardware (by controlling physical access) does take out a
whole class of attack vectors.

But, seriously, such discussion has the tendency to quickly get t
theoretical (thus losing practical significance). For example, would we
not also need to prevent the (userspace) superuser from being able to run
arbitrary executables that can modify firmware? Okay, let's say we have
a kernelspace infrastructure of verifying cryptographic signatures on
binaries before executing them ... but how practical/usable is this?
How practically/universally applicable is a system whose security derives
from keeping machines behind locked doors and protected by incorruptible,
armed guard?

Overall, I tend to be unenthusiastic about most schemes that claim to
have solved the user-kernel security problem (with no loss of usability/
practicality).
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NFS4 authentification / fsuid

[Satyam Sharma]

On Thu, 6 Sep 2007, J. Bruce Fields wrote:
> 
> On Thu, Sep 06, 2007 at 01:59:50PM +0530, Satyam Sharma wrote:
> > Oh and btw, note that we're talking of the (lack of) security of a
> > "running kernel" here -- because across reboots, there is /really/
> > *absolutely* no such thing as "kernelspace security" because the superuser
> > will simply switch the vmlinuz itself ...
> 
> Well, the machine could be booting from cdrom, and could live in a
> locked machine room.

And how is this different from the "trusted tamperproof hardware"
solution I proposed earlier? From an attack vector p.o.v. they are both
precisely the same -- both of them are designed to prevent the attacker
from gaining unfettered access to system hardware, hmm?

Oh, actually, if past history is anything to go by, then your scheme
is provably weaker. Security systems are invariably always broken at
their weakest link, which is invariably always the human/social element,
and your scheme derives its security by relying on *social* element.

To elaborate my point, what prevents me from bribing / torturing /
blackmailing whoever owns the key to that locked server room and ...

The attack is "non-technical", but hey, so was your security :-)


> Or people with root on a virtual host don't
> necessarily have the ability to replace the kernel for that host.

Again, you're restricting physical access ... but okay, this is a slightly
more plausible solution (but one that applies to only a *specific* kind of
situation).


Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: 2.6.23-rc6-mm1

[Satyam Sharma]

On Tue, 18 Sep 2007, Andrew Morton wrote:

> On Tue, 18 Sep 2007 14:43:48 +0530 Kamalesh Babulal <[EMAIL PROTECTED]> wrote:
> 
> > Andrew Morton wrote:
> > > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.23-rc6/2.6.23-rc6-mm1/
> > >
> > > 2.6.23-rc6-mm1 is a 29MB diff against 2.6.23-rc6.
> > >
> > >   
> > 
> > 
> > Hi Andrew,
> > 
> > The 2.6.23-rc6-mm1build fails at
> > 
> >   CC  drivers/pci/hotplug/rpadlpar_core.o
> >   CC  drivers/pci/hotplug/rpadlpar_sysfs.o
> > drivers/pci/hotplug/rpadlpar_sysfs.c:132: error: unknown field `name' 
> > specified in initializer
> > drivers/pci/hotplug/rpadlpar_sysfs.c: In function `dlpar_sysfs_init':
> > drivers/pci/hotplug/rpadlpar_sysfs.c:142: error: structure has no member 
> > named `name'
> > make[3]: *** [drivers/pci/hotplug/rpadlpar_sysfs.o] Error 1
> > make[2]: *** [drivers/pci/hotplug] Error 2
> > make[1]: *** [drivers/pci] Error 2
> > make: *** [drivers] Error 2
> > 
> 
> Probably this:
> 
> --- a/drivers/pci/hotplug/rpadlpar_sysfs.c~a
> +++ a/drivers/pci/hotplug/rpadlpar_sysfs.c
> @@ -129,8 +129,7 @@ struct kobj_type ktype_dlpar_io = {
>  };
>  
>  struct kset dlpar_io_kset = {
> - .kobj = {.name = DLPAR_KOBJ_NAME,
> -  .ktype = _dlpar_io,
> + .kobj = {.ktype = _dlpar_io,
>.parent = _hotplug_slots_subsys.kobj},
>   .ktype = _dlpar_io,
>  };
> _
> 
> But I'm fed up with fixing that patch.  It's Greg's turn.

But wait ... isn't that a statically-allocated kobject, which were
supposed to be "naughty" in the first place?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/2] unify DMA_..BIT_MASK definitions: v2

[Satyam Sharma]

On Tue, 18 Sep 2007, Borislav Petkov wrote:

> These patches remove redundant DMA_..BIT_MASK definitions across
> two drivers. In this version of the patches, the computation
> of the majority of the bitmasks is done by the compiler.
> 
> Signed-off-by: Borislav Petkov <[EMAIL PROTECTED]>
> Cc: Jeremy Fitzhardinge <[EMAIL PROTECTED]>
> Cc: Muli Ben-Yehuda <[EMAIL PROTECTED]>

Just a friendly note to whoever (Jeremy?) picks up these patches --
kindly fold all three into a single git commit. No point introducing
new compile warnings to the kernel build process for no good reason
(and some archs do set -Werror in their top level Makefiles, even).


> ---
>  23-rc6/include/linux/dma-mapping.h |   23 +--
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> Index: b/23-rc6/include/linux/dma-mapping.h
> ===
> --- a/23-rc6/include/linux/dma-mapping.h  2007-09-17 17:48:20.0 
> +0200
> +++ b/23-rc6/include/linux/dma-mapping.h  2007-09-18 08:11:19.0 
> +0200
  ^

And whoever picks these up is gonna have trouble applying them too.

Borislav, care to have another shot at this ?

Thanks,
Satyam


> @@ -13,16 +13,19 @@
>   DMA_NONE = 3,
>  };
>  
> -#define DMA_64BIT_MASK   0xULL
> -#define DMA_48BIT_MASK   0xULL
> -#define DMA_40BIT_MASK   0x00ffULL
> -#define DMA_39BIT_MASK   0x007fULL
> -#define DMA_32BIT_MASK   0xULL
> -#define DMA_31BIT_MASK   0x7fffULL
> -#define DMA_30BIT_MASK   0x3fffULL
> -#define DMA_29BIT_MASK   0x1fffULL
> -#define DMA_28BIT_MASK   0x0fffULL
> -#define DMA_24BIT_MASK   0x00ffULL
> +#define DMA_BIT_MASK(n)  ((1ULL<<(n))-1)
> +
> +#define DMA_64BIT_MASK   ~0ULL
> +#define DMA_48BIT_MASK   DMA_BIT_MASK(48)
> +#define DMA_40BIT_MASK   DMA_BIT_MASK(40)
> +#define DMA_39BIT_MASK   DMA_BIT_MASK(39)
> +#define DMA_35BIT_MASK   DMA_BIT_MASK(35)
> +#define DMA_32BIT_MASK   DMA_BIT_MASK(32)
> +#define DMA_31BIT_MASK   DMA_BIT_MASK(31)
> +#define DMA_30BIT_MASK   DMA_BIT_MASK(30)
> +#define DMA_29BIT_MASK   DMA_BIT_MASK(29)
> +#define DMA_28BIT_MASK   DMA_BIT_MASK(28)
> +#define DMA_24BIT_MASK   DMA_BIT_MASK(24)
>  
>  static inline int valid_dma_direction(int dma_direction)
>  {
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/2] unify DMA_..BIT_MASK definitions: v1

[Satyam Sharma]

Hi Borislav,


On Tue, 18 Sep 2007, Borislav Petkov wrote:

> On Mon, Sep 17, 2007 at 11:01:21PM -0700, Jeremy Fitzhardinge wrote:
> > Muli Ben-Yehuda wrote:
> > >> +#define DMA_64BIT_MASK  DMA_BIT_MASK(64)
> > >> 
> > >
> > > This one does not do what you mean. You need an explicit mask or a
> > > ~0ULL here.
> > 
> > Yeah, I was just about to comment on it.  Its possible the compiler
> > might decide to shift by x%64 = 0.
> > 
> > J
> ups, i knew that this might be a corner/boundary case. Thanks, updated patches
> follow...

Please fold all three patches into a single patch in the updated series,
otherwise git bisecters falling in between these patches will see the
"redefinition ... previous definition was here" warnings of gcc ...


Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/2] unify DMA_..BIT_MASK definitions: v1

[Satyam Sharma]

Hi Borislav,


On Tue, 18 Sep 2007, Borislav Petkov wrote:

 On Mon, Sep 17, 2007 at 11:01:21PM -0700, Jeremy Fitzhardinge wrote:
  Muli Ben-Yehuda wrote:
   +#define DMA_64BIT_MASK  DMA_BIT_MASK(64)
   
  
   This one does not do what you mean. You need an explicit mask or a
   ~0ULL here.
  
  Yeah, I was just about to comment on it.  Its possible the compiler
  might decide to shift by x%64 = 0.
  
  J
 ups, i knew that this might be a corner/boundary case. Thanks, updated patches
 follow...

Please fold all three patches into a single patch in the updated series,
otherwise git bisecters falling in between these patches will see the
redefinition ... previous definition was here warnings of gcc ...


Satyam
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/2] unify DMA_..BIT_MASK definitions: v2

[Satyam Sharma]

On Tue, 18 Sep 2007, Borislav Petkov wrote:

 These patches remove redundant DMA_..BIT_MASK definitions across
 two drivers. In this version of the patches, the computation
 of the majority of the bitmasks is done by the compiler.
 
 Signed-off-by: Borislav Petkov [EMAIL PROTECTED]
 Cc: Jeremy Fitzhardinge [EMAIL PROTECTED]
 Cc: Muli Ben-Yehuda [EMAIL PROTECTED]

Just a friendly note to whoever (Jeremy?) picks up these patches --
kindly fold all three into a single git commit. No point introducing
new compile warnings to the kernel build process for no good reason
(and some archs do set -Werror in their top level Makefiles, even).


 ---
  23-rc6/include/linux/dma-mapping.h |   23 +--
  1 file changed, 13 insertions(+), 10 deletions(-)
 
 Index: b/23-rc6/include/linux/dma-mapping.h
 ===
 --- a/23-rc6/include/linux/dma-mapping.h  2007-09-17 17:48:20.0 
 +0200
 +++ b/23-rc6/include/linux/dma-mapping.h  2007-09-18 08:11:19.0 
 +0200
  ^

And whoever picks these up is gonna have trouble applying them too.

Borislav, care to have another shot at this ?

Thanks,
Satyam


 @@ -13,16 +13,19 @@
   DMA_NONE = 3,
  };
  
 -#define DMA_64BIT_MASK   0xULL
 -#define DMA_48BIT_MASK   0xULL
 -#define DMA_40BIT_MASK   0x00ffULL
 -#define DMA_39BIT_MASK   0x007fULL
 -#define DMA_32BIT_MASK   0xULL
 -#define DMA_31BIT_MASK   0x7fffULL
 -#define DMA_30BIT_MASK   0x3fffULL
 -#define DMA_29BIT_MASK   0x1fffULL
 -#define DMA_28BIT_MASK   0x0fffULL
 -#define DMA_24BIT_MASK   0x00ffULL
 +#define DMA_BIT_MASK(n)  ((1ULL(n))-1)
 +
 +#define DMA_64BIT_MASK   ~0ULL
 +#define DMA_48BIT_MASK   DMA_BIT_MASK(48)
 +#define DMA_40BIT_MASK   DMA_BIT_MASK(40)
 +#define DMA_39BIT_MASK   DMA_BIT_MASK(39)
 +#define DMA_35BIT_MASK   DMA_BIT_MASK(35)
 +#define DMA_32BIT_MASK   DMA_BIT_MASK(32)
 +#define DMA_31BIT_MASK   DMA_BIT_MASK(31)
 +#define DMA_30BIT_MASK   DMA_BIT_MASK(30)
 +#define DMA_29BIT_MASK   DMA_BIT_MASK(29)
 +#define DMA_28BIT_MASK   DMA_BIT_MASK(28)
 +#define DMA_24BIT_MASK   DMA_BIT_MASK(24)
  
  static inline int valid_dma_direction(int dma_direction)
  {
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: 2.6.23-rc6-mm1

[Satyam Sharma]

On Tue, 18 Sep 2007, Andrew Morton wrote:

 On Tue, 18 Sep 2007 14:43:48 +0530 Kamalesh Babulal [EMAIL PROTECTED] wrote:
 
  Andrew Morton wrote:
   ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.23-rc6/2.6.23-rc6-mm1/
  
   2.6.23-rc6-mm1 is a 29MB diff against 2.6.23-rc6.
  
 
  snip
  
  Hi Andrew,
  
  The 2.6.23-rc6-mm1build fails at
  
CC  drivers/pci/hotplug/rpadlpar_core.o
CC  drivers/pci/hotplug/rpadlpar_sysfs.o
  drivers/pci/hotplug/rpadlpar_sysfs.c:132: error: unknown field `name' 
  specified in initializer
  drivers/pci/hotplug/rpadlpar_sysfs.c: In function `dlpar_sysfs_init':
  drivers/pci/hotplug/rpadlpar_sysfs.c:142: error: structure has no member 
  named `name'
  make[3]: *** [drivers/pci/hotplug/rpadlpar_sysfs.o] Error 1
  make[2]: *** [drivers/pci/hotplug] Error 2
  make[1]: *** [drivers/pci] Error 2
  make: *** [drivers] Error 2
  
 
 Probably this:
 
 --- a/drivers/pci/hotplug/rpadlpar_sysfs.c~a
 +++ a/drivers/pci/hotplug/rpadlpar_sysfs.c
 @@ -129,8 +129,7 @@ struct kobj_type ktype_dlpar_io = {
  };
  
  struct kset dlpar_io_kset = {
 - .kobj = {.name = DLPAR_KOBJ_NAME,
 -  .ktype = ktype_dlpar_io,
 + .kobj = {.ktype = ktype_dlpar_io,
.parent = pci_hotplug_slots_subsys.kobj},
   .ktype = ktype_dlpar_io,
  };
 _
 
 But I'm fed up with fixing that patch.  It's Greg's turn.

But wait ... isn't that a statically-allocated kobject, which were
supposed to be naughty in the first place?
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NFS4 authentification / fsuid

[Satyam Sharma]

On Fri, 7 Sep 2007, J. Bruce Fields wrote:
 
 On Fri, Sep 07, 2007 at 01:32:52AM +0200, Trond Myklebust wrote:
  Sorry. Of course, you have to copy the entire /lib, etc. onto the tmpfs,
  but you get the gist
  
  The point is that it is easy to subvert userspace if you have enough
  privileges. In the above example it may not be entirely undetectable,
  but who here is running a script on every login to check that / is
  indeed uncompromised?
 
 I suppose this is the motivation for things like the secure attention
 key?
 
 But I'm most curious actually about to what degree the kernel itself is
 vulnerable to root (without a reboot).  Is disabling /dev/kmem and
 module-loading in theory enough?

No, not in theory, not in practice. But yeah, restricting an attacker's
ability to hack hardware (by controlling physical access) does take out a
whole class of attack vectors.

But, seriously, such discussion has the tendency to quickly get t
theoretical (thus losing practical significance). For example, would we
not also need to prevent the (userspace) superuser from being able to run
arbitrary executables that can modify firmware? Okay, let's say we have
a kernelspace infrastructure of verifying cryptographic signatures on
binaries before executing them ... but how practical/usable is this?
How practically/universally applicable is a system whose security derives
from keeping machines behind locked doors and protected by incorruptible,
armed guard?

Overall, I tend to be unenthusiastic about most schemes that claim to
have solved the user-kernel security problem (with no loss of usability/
practicality).
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NFS4 authentification / fsuid

[Satyam Sharma]

On Thu, 6 Sep 2007, J. Bruce Fields wrote:
 
 On Thu, Sep 06, 2007 at 01:59:50PM +0530, Satyam Sharma wrote:
  Oh and btw, note that we're talking of the (lack of) security of a
  running kernel here -- because across reboots, there is /really/
  *absolutely* no such thing as kernelspace security because the superuser
  will simply switch the vmlinuz itself ...
 
 Well, the machine could be booting from cdrom, and could live in a
 locked machine room.

And how is this different from the trusted tamperproof hardware
solution I proposed earlier? From an attack vector p.o.v. they are both
precisely the same -- both of them are designed to prevent the attacker
from gaining unfettered access to system hardware, hmm?

Oh, actually, if past history is anything to go by, then your scheme
is provably weaker. Security systems are invariably always broken at
their weakest link, which is invariably always the human/social element,
and your scheme derives its security by relying on *social* element.

To elaborate my point, what prevents me from bribing / torturing /
blackmailing whoever owns the key to that locked server room and ...

The attack is non-technical, but hey, so was your security :-)


 Or people with root on a virtual host don't
 necessarily have the ability to replace the kernel for that host.

Again, you're restricting physical access ... but okay, this is a slightly
more plausible solution (but one that applies to only a *specific* kind of
situation).


Satyam
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NFS4 authentification / fsuid

[Satyam Sharma]

On Thu, 6 Sep 2007, Kyle Moffett wrote:
 
 On Sep 06, 2007, at 19:35:14, Trond Myklebust wrote:
  
  On Thu, 2007-09-06 at 19:30 -0400, Kyle Moffett wrote:
   
   On Sep 06, 2007, at 11:06:16, J. Bruce Fields wrote:
The question of how to protect against someone with *physical*
   ^^^
access certainly is more difficult, but surely that's a separate
^^

problem.
   
   Actually, that's a fairly simple problem (barring disassembling the system


   and attaching a hardware debugger).  You encrypt the root filesystem and
   require a password to boot (See: LUKS).  Debian has built-in support for
   installing onto fs-on-LVM-on-crypt-on-RAID, and it works quite well on all
   the laptops I use regularly.  It's not even much of a speed penalty; once
   you take the overhead of hitting a 5400RPM laptop drive you can chew
   thousands of cycles of CPU without anybody noticing (much).  Then all you
   have to do is burn a copy of your /boot with bootloader onto some
   read-only media (like a finalized CDROM/DVDROM) and you're set to go.
  
  Disconnect battery, and watch boot password go 'poof!'.
 
 Umm, I did say encrypt the root filesystem, didn't I?  Booting my laptops
  ^^^

The whole *point* here is to secure against physical access -- then how
can you assume barring disassembling the system? If you're not
considering attacks such as those, then how _are_ you solving the
physical access problem in the first place? :-)


 this way follows this procedure:
  1) Enter BIOS boot menu
  2) Insert /boot CDROM
  3) Select the CDROM entry
  4) Wait for kernel to start and run through initramfs
  5) Type password into the initramfs prompt so that it can DECRYPT THE ROOT
 FILESYSTEM
  6) Continue to boot the system.
 
 Under this setup, tinkering with my BIOS does virtually nothing; the only
 avenues of attack are strictly of the Install a hardware keylogger variety.

Doesn't flashing/replacing your BIOS firmware/chip count as tinkering?
Then I don't really need a hardware keylogger, do I ...
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NFS4 authentification / fsuid

[Satyam Sharma]

On Fri, 7 Sep 2007, Kyle Moffett wrote:
 
 So you can't draw any relationships between Protect the end-user with
 Protect the device FROM the end-user, the former can be done very reliably
   ^^^ *attacker*

 to whatever level of risk-reduction you need and the latter can't practically
 be done at all.

Well, you're the one who called solving the physical access problem
easy here ... :-)
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Reduce __print_symbol/sprint_symbol stack usage.

[Satyam Sharma]

Hi Gilboa,


On Sat, 15 Sep 2007, Gilboa Davara wrote:
 
 This is my second stab at solving the stack over flow due to
 dump_strace when close to stack-overflow is detected by do_IRQ problem.
 (Hopefully) this patch is creates less noise then the previous one.
 
 [snip]
  I'll try and create an option 2 (static allocation, minimal locking)
  patch and post ASAP.
  Hopefully it'll fare better. (While keeping the current interface intact
  and reducing the damage/noise)
 
 - Gilboa
 
 --- linux-2.6/kernel/kallsyms.orig2007-09-15 11:46:54.0 +0300
 +++ linux-2.6/kernel/kallsyms.c   2007-09-15 21:06:55.0 +0300
 @@ -306,13 +306,14 @@ int lookup_symbol_attrs(unsigned long ad
   return lookup_module_symbol_attrs(addr, size, offset, modname, name);
  }
  
 -/* Look up a kernel symbol and return it in a text buffer. */
 -int sprint_symbol(char *buffer, unsigned long address)
 +/* Internal version:
 +   Look up a kernel symbol and module name and return them to the
 + caller's buffer/namebuf buffers. */

/*
 * ...
 * ...
 */

is the general coding style here ...

 +int __sprint_symbol(char *buffer, char *namebuf, unsigned long address)
  {
 - char *modname;
 - const char *name;
   unsigned long offset, size;
 - char namebuf[KSYM_NAME_LEN];
 + const char *name;
 + char *modname;
  
   name = kallsyms_lookup(address, size, offset, modname, namebuf);
   if (!name)
 @@ -325,14 +326,35 @@ int sprint_symbol(char *buffer, unsigned
   return sprintf(buffer, %s+%#lx/%#lx, name, offset, size);
  }
  
 +/* Exported version:
 +   Look up a kernel symbol and return it in a text buffer. */

ditto.

 +int sprint_symbol(char *buffer, unsigned long address)
 +{
 + char namebuf[KSYM_NAME_LEN];

Hmm, don't we intend to push this array out of the stack too?

+   static char namebuf[KSYM_NAME_LEN];
+   static DEFINE_SPINLOCK(namebuf_lock);

here ?

 +
 + return __sprint_symbol(buffer, namebuf, address);

And you'd need to wrap spin_lock_irqsave()/spin_unlock_irqrestore()
around this call.

 +}


 +static DEFINE_SPINLOCK(symbol_lock);

Try to keep the declarations of a lock, and the data that it protects,
close together. Since this lock is being used to protect buffer, it
makes sense to ...


  /* Look up a kernel symbol and print it to the kernel messages. */
  void __print_symbol(const char *fmt, unsigned long address)
  {
 - char buffer[KSYM_SYMBOL_LEN];
 + /* Use static buffers instead of char array to reduce
 +  stack footprint in i386/4KSTACKS.
 +  Buffers must be protected against re-entry. */
 + static char namebuf[KSYM_NAME_LEN];
 + static char buffer[KSYM_SYMBOL_LEN];

... have it:

+   static DEFINE_SPINLOCK(buffer_lock);

here (note the name that exactly describes what the lock protects).

And the namebuf array isn't required here, it's already there in
sprint_symbol(), which you can call from ...

 + unsigned long flags;
 +
  
 - sprint_symbol(buffer, address);
 + spin_lock_irqsave(symbol_lock, flags);
 +
 + __sprint_symbol(buffer, namebuf, address);

here ... sprint_symbol() ?

   printk(fmt, buffer);
 +
 + spin_unlock_irqrestore(symbol_lock, flags);

But I still don't much like this :-(

More importantly, if a panic occurs *below* this callchain (and let's
say we ended up in this callchain because somebody put in a dump_stack()
somewhere for debugging purposes), then we'd have a deadlock on our hands,
and nothing gets printed for that panic.

I don't know who maintains this part of kernel code, but you can try
resubmitting (with the changes suggested above) to someone appropriate ...


Satyam
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: iso9660 vs udf

[Satyam Sharma]

Hi,

On Tue, 18 Sep 2007, Jan Kara wrote:
 
  Today I got a CD. MacOS does not mount it and Linux does not
  mount it without an explicit filesystemtype option.
  That is,
  # mount /dev/hdc /dir -t iso9660
  works fine, but
  # mount /dev/hdc /dir
  mount: you didn't specify a filesystem type for /dev/hdc
 I will try type udf
  mount: wrong fs type, bad option, bad superblock on /dev/hdc,
 missing codepage or other error
 In some cases useful info is found in syslog - try
 dmesg | tail  or so
  # dmesg | tail
  UDF-fs INFO UDF 0.9.8.1 (2004/29/09) Mounting volume 'Wisk1956-82', 
  timestamp 2006/03/07 16:26 (1078)
  udf: udf_read_inode(ino 547) failed !bh
  UDF-fs: Error in udf_iget, block=1, partition=1

That comes from udf_fill_super() but which shouldn't have been called
in the first place ...

  Google gave me half a dozen other people that mentioned the same
  problem (with the same inode 547). Clearly some CD mastering software
  produces a format that Linux and MacOS do not handle easily.
  
  One result of this letter will be that people with the same problem
  learn via Google that using the -t iso9660 option may help.
  
  What goes wrong on the mount side is that when it hesitates between
  iso9660 and udf it decides for udf when seeing NSR02.
  Maybe the heuristics in mount should be tuned.
   Yes, this seems like a mount problem but you should contact mount
 maintainer for that... I guess hardly anyone will help you with this on
 this list.
 
  On the other hand, this filesystem announces itself as UDF
  (CD-RTOS CD-BRIDGE CDUDF File System - Adaptec Inc),
  perhaps the kernel code should be more robust.

Could you send the complete dmesg log, and what you mean with filesystem/
kernel (incorrectly?) announcing it as UDF here ... I agree with Jan,
this sounds like an issue with mount(8) to me.

  If anybody feels responsible for mount and/or this kernel area
  we might discuss.
   I'm kind of taking care about UDF in kernel. What do you find
 inappropriate on the kernel reaction? You mean we should produce some
 better error message into the log?
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: iso9660 vs udf

[Satyam Sharma]

Hi Andries,


On Wed, 19 Sep 2007, Andries E. Brouwer wrote:
 
 On Wed, Sep 19, 2007 at 05:48:28AM +0530, Satyam Sharma wrote:
 
On the other hand, this filesystem announces itself as UDF
(CD-RTOS CD-BRIDGE CDUDF File System - Adaptec Inc),
perhaps the kernel code should be more robust.
  
  Could you send the complete dmesg log, and what you mean with filesystem/
  kernel (incorrectly?) announcing it as UDF here ... I agree with Jan,
  this sounds like an issue with mount(8) to me.
 
 You already got the relevant part of the dmesg log. Slightly more below.

 Failed mount:
 UDF-fs INFO UDF 0.9.8.1 (2004/29/09) Mounting volume 'Wisk1956-82', timestamp 
 2006/03/07 16:26 (1078)
 udf: udf_read_inode(ino 547) failed !bh
 UDF-fs: Error in udf_iget, block=1, partition=1

Ok, like said, this comes from udf_fill_super(), but which shouldn't
have been called for this CD in the first place -- i.e. mount(8) shouldn't
have tried to mount a non-UDF filesystem as UDF (unless explicitly asked
as such). I was actually asking for the logs explaining why you thought
the _kernel_ incorrectly announced it as an UDF filesystem.

Hmm ... those CD-RTOS, CD-BRIDGE and CDUDF File System - Adaptec Inc
bits are not dmesg output, are they? Looks like hwinfo --cdrom or
isoinfo or some such.

 I think the filesystem can be treated both as iso9660 and as udf,
 at least that is what I seem to recall CD-BRIDGE means.  Thus,
 if the kernel cannot mount it as udf, I think it is a kernel flaw.
 Given that kernel flaw, and the fact that mounting as iso9660 works,
 mount(8) could work around the kernel problem by guessing iso9660.
 But maybe we should first try to fix the kernel.

I don't think that is what CD-BRIDGE means -- so no kernel flaw :-)
What happened here is simply that in the absence of a -t option,
mount(8) defaulted (probably due to incorrect heuristics?) to UDF for
some reason, thereby obviously failing. I don't know who maintains
mount(8) / util-linux package, or do distributions have their own
maintainers these days (?)


Satyam
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] powerpc: Avoid pointless WARN_ON(irqs_disabled()) from panic codepath

[Satyam Sharma]

On Tue, 18 Sep 2007, Satyam Sharma wrote:
> 
> > [ cut here ]
> > Badness at arch/powerpc/kernel/smp.c:202
> 
> comes when smp_call_function_map() has been called with irqs disabled,
> which is illegal. However, there is a special case, the panic() codepath,
> when we do not want to warn about this -- warning at that time is pointless
> anyway, and only serves to scroll away the *real* cause of the panic and
> distracts from the real bug.

BTW arch/ppc/ has same issue, but that's gonna be removed by next year
anyways, so I think there's no point making a patch for that (?)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: 2.6.23-rc4-mm1 OOPS in forcedeth?

[Satyam Sharma]

On Mon, 17 Sep 2007, Denis V. Lunev wrote:
> Dhaval Giani wrote:
> > On Thu, Sep 13, 2007 at 11:51:33PM -0400, Andrew James Wade wrote:

> >> EIP: [] tcp_rto_min+0xb/0x15 SS:ESP 0068:c0596dec

As Vlad Yasevich mentioned, this one is already fixed in 23-rc6.

The forcedeth oops is unrelated, but multiple people have reported that
same oops now -- adding Manfred Spraul to CC. [ original thread is at:
http://lkml.org/lkml/2007/9/1/115 ]
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] powerpc: Avoid pointless WARN_ON(irqs_disabled()) from panic codepath

[Satyam Sharma]

> [ cut here ]
> Badness at arch/powerpc/kernel/smp.c:202

comes when smp_call_function_map() has been called with irqs disabled,
which is illegal. However, there is a special case, the panic() codepath,
when we do not want to warn about this -- warning at that time is pointless
anyway, and only serves to scroll away the *real* cause of the panic and
distracts from the real bug.

* So let's extract the WARN_ON() from smp_call_function_map() into all its
  callers -- smp_call_function() and smp_call_function_single()

* Also, introduce another caller of smp_call_function_map(), namely
  __smp_call_function() (and make smp_call_function() a wrapper over this)
  which does *not* warn about disabled irqs

* Use this __smp_call_function() from the panic codepath's smp_send_stop()

We also end having to move code of smp_send_stop() below the definition
of __smp_call_function().

Signed-off-by: Satyam Sharma <[EMAIL PROTECTED]>

---

Untested (not even compile-tested) patch.
Could someone point me to ppc32/64 cross-compilers for i386?

 arch/powerpc/kernel/smp.c |   27 ++-
 1 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 1ea4316..b24dcba 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -152,11 +152,6 @@ static void stop_this_cpu(void *dummy)
;
 }
 
-void smp_send_stop(void)
-{
-   smp_call_function(stop_this_cpu, NULL, 1, 0);
-}
-
 /*
  * Structure and data for smp_call_function(). This is designed to minimise
  * static memory requirements. It also looks cleaner.
@@ -198,9 +193,6 @@ int smp_call_function_map(void (*func) (void *info), void 
*info, int nonatomic,
int cpu;
u64 timeout;
 
-   /* Can deadlock when called with interrupts disabled */
-   WARN_ON(irqs_disabled());
-
if (unlikely(smp_ops == NULL))
return ret;
 
@@ -270,10 +262,19 @@ int smp_call_function_map(void (*func) (void *info), void 
*info, int nonatomic,
return ret;
 }
 
+static int __smp_call_function(void (*func)(void *info), void *info,
+  int nonatomic, int wait)
+{
+   return smp_call_function_map(func,info,nonatomic,wait,cpu_online_map);
+}
+
 int smp_call_function(void (*func) (void *info), void *info, int nonatomic,
int wait)
 {
-   return smp_call_function_map(func,info,nonatomic,wait,cpu_online_map);
+   /* Can deadlock when called with interrupts disabled */
+   WARN_ON(irqs_disabled());
+
+   return __smp_call_function(func, info, nonatomic, wait);
 }
 EXPORT_SYMBOL(smp_call_function);
 
@@ -283,6 +284,9 @@ int smp_call_function_single(int cpu, void (*func) (void 
*info), void *info, int
cpumask_t map = CPU_MASK_NONE;
int ret = 0;
 
+   /* Can deadlock when called with interrupts disabled */
+   WARN_ON(irqs_disabled());
+
if (!cpu_online(cpu))
return -EINVAL;
 
@@ -299,6 +303,11 @@ int smp_call_function_single(int cpu, void (*func) (void 
*info), void *info, int
 }
 EXPORT_SYMBOL(smp_call_function_single);
 
+void smp_send_stop(void)
+{
+   __smp_call_function(stop_this_cpu, NULL, 1, 0);
+}
+
 void smp_call_function_interrupt(void)
 {
void (*func) (void *info);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] modpost: detect unterminated device id lists

[Satyam Sharma]

On Sun, 16 Sep 2007, Andrew Morton wrote:

> On Mon, 17 Sep 2007 05:54:45 +0530 "Satyam Sharma" <[EMAIL PROTECTED]> wrote:
> 
> > On 9/17/07, Andrew Morton <[EMAIL PROTECTED]> wrote:
> > >
> > > I'm getting this:
> > >
> > > rusb2/pvrusb2: struct usb_device_id is 20 bytes.  The last of 3 is:
> > > 0x03 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
> > > 0x00 0x00 0x00 0x00 0x00
> > > FATAL: drivers/media/video/pvrusb2/pvrusb2: struct usb_device_id is not 
> > > terminated
> > > with a NULL entry!
> > >
> > > ("rusb2/pvrusb2" ??)
> > 
> > Hmm? Are you sure you didn't see any "drivers/media/video/pv" before the
> > "rusb2/pvrusb2" bit?
> 
> Fairly.  I looked twice.

"drivers/media/video/pvrusb2/pvrusb2" comes out correctly here ...


> > Looking at Kees' patch (and the existing code), I've no
> > clue how/why this should happen ... will try to reproduce here ...
> > 
> > 
> > > but:
> > >
> > > struct usb_device_id pvr2_device_table[] = {
> > > [PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
> > > [PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
> > > { USB_DEVICE(0, 0) },
> > > };
> > >
> > > looks OK?
> > >
> > > Using plain old "{ }" shut the warning up.
> > 
> > USB_DEVICE(0, 0) is not empty termination, actually, and this looks like
> > a genuine bug caught by the patch. As that dump shows, USB_DEVICE(0, 0)
> > assigns "0x03 0x00" (in little endian) to usb_device_id.match_flags. And
> > I don't think the USB code treats such an entry as an empty entry (?)
> > 
> > Interestingly, the "USB_DEVICE(0, 0)" thing is absent from latest -git
> > tree and also in my copy of 23-rc4-mm1 -- so this looks like something
> > you must've merged recently.
> 
> git-dvb very carefully does
> 
> --- a/drivers/media/video/pvrusb2/pvrusb2-hdw.c~git-dvb
> +++ a/drivers/media/video/pvrusb2/pvrusb2-hdw.c
> @@ -44,7 +44,7 @@
>  struct usb_device_id pvr2_device_table[] = {
>   [PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
>   [PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
> -   { }
> +   { USB_DEVICE(0, 0) },
> };
>
> MODULE_DEVICE_TABLE(usb, pvr2_device_table);

Ok, this is a false positive indeed, the core USB code does in fact
treat such an entry as an empty entry (usb_match_id() tests only the
.idVendor, .bDeviceClass, .bInterfaceClass and .driver_info members
for non-zero and not the .match_flags member).

However, a quick-grep-and-glance tells us that none of the other 2213
occurrences of USB_DEVICE() in the tree ever do this "(0,0)" thing,
so it does make sense to change this one to a simple "{ }" as well --
that's clearer style anyway, and the "standard" way to empty-terminate
in the rest of the tree, if nothing else.


Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] modpost: detect unterminated device id lists

[Satyam Sharma]

On Sun, 16 Sep 2007, Andrew Morton wrote:

 On Mon, 17 Sep 2007 05:54:45 +0530 Satyam Sharma [EMAIL PROTECTED] wrote:
 
  On 9/17/07, Andrew Morton [EMAIL PROTECTED] wrote:
  
   I'm getting this:
  
   rusb2/pvrusb2: struct usb_device_id is 20 bytes.  The last of 3 is:
   0x03 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
   0x00 0x00 0x00 0x00 0x00
   FATAL: drivers/media/video/pvrusb2/pvrusb2: struct usb_device_id is not 
   terminated
   with a NULL entry!
  
   (rusb2/pvrusb2 ??)
  
  Hmm? Are you sure you didn't see any drivers/media/video/pv before the
  rusb2/pvrusb2 bit?
 
 Fairly.  I looked twice.

drivers/media/video/pvrusb2/pvrusb2 comes out correctly here ...


  Looking at Kees' patch (and the existing code), I've no
  clue how/why this should happen ... will try to reproduce here ...
  
  
   but:
  
   struct usb_device_id pvr2_device_table[] = {
   [PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
   [PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
   { USB_DEVICE(0, 0) },
   };
  
   looks OK?
  
   Using plain old { } shut the warning up.
  
  USB_DEVICE(0, 0) is not empty termination, actually, and this looks like
  a genuine bug caught by the patch. As that dump shows, USB_DEVICE(0, 0)
  assigns 0x03 0x00 (in little endian) to usb_device_id.match_flags. And
  I don't think the USB code treats such an entry as an empty entry (?)
  
  Interestingly, the USB_DEVICE(0, 0) thing is absent from latest -git
  tree and also in my copy of 23-rc4-mm1 -- so this looks like something
  you must've merged recently.
 
 git-dvb very carefully does
 
 --- a/drivers/media/video/pvrusb2/pvrusb2-hdw.c~git-dvb
 +++ a/drivers/media/video/pvrusb2/pvrusb2-hdw.c
 @@ -44,7 +44,7 @@
  struct usb_device_id pvr2_device_table[] = {
   [PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
   [PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
 -   { }
 +   { USB_DEVICE(0, 0) },
 };

 MODULE_DEVICE_TABLE(usb, pvr2_device_table);

Ok, this is a false positive indeed, the core USB code does in fact
treat such an entry as an empty entry (usb_match_id() tests only the
.idVendor, .bDeviceClass, .bInterfaceClass and .driver_info members
for non-zero and not the .match_flags member).

However, a quick-grep-and-glance tells us that none of the other 2213
occurrences of USB_DEVICE() in the tree ever do this (0,0) thing,
so it does make sense to change this one to a simple { } as well --
that's clearer style anyway, and the standard way to empty-terminate
in the rest of the tree, if nothing else.


Satyam
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] powerpc: Avoid pointless WARN_ON(irqs_disabled()) from panic codepath

[Satyam Sharma]

 [ cut here ]
 Badness at arch/powerpc/kernel/smp.c:202

comes when smp_call_function_map() has been called with irqs disabled,
which is illegal. However, there is a special case, the panic() codepath,
when we do not want to warn about this -- warning at that time is pointless
anyway, and only serves to scroll away the *real* cause of the panic and
distracts from the real bug.

* So let's extract the WARN_ON() from smp_call_function_map() into all its
  callers -- smp_call_function() and smp_call_function_single()

* Also, introduce another caller of smp_call_function_map(), namely
  __smp_call_function() (and make smp_call_function() a wrapper over this)
  which does *not* warn about disabled irqs

* Use this __smp_call_function() from the panic codepath's smp_send_stop()

We also end having to move code of smp_send_stop() below the definition
of __smp_call_function().

Signed-off-by: Satyam Sharma [EMAIL PROTECTED]

---

Untested (not even compile-tested) patch.
Could someone point me to ppc32/64 cross-compilers for i386?

 arch/powerpc/kernel/smp.c |   27 ++-
 1 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 1ea4316..b24dcba 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -152,11 +152,6 @@ static void stop_this_cpu(void *dummy)
;
 }
 
-void smp_send_stop(void)
-{
-   smp_call_function(stop_this_cpu, NULL, 1, 0);
-}
-
 /*
  * Structure and data for smp_call_function(). This is designed to minimise
  * static memory requirements. It also looks cleaner.
@@ -198,9 +193,6 @@ int smp_call_function_map(void (*func) (void *info), void 
*info, int nonatomic,
int cpu;
u64 timeout;
 
-   /* Can deadlock when called with interrupts disabled */
-   WARN_ON(irqs_disabled());
-
if (unlikely(smp_ops == NULL))
return ret;
 
@@ -270,10 +262,19 @@ int smp_call_function_map(void (*func) (void *info), void 
*info, int nonatomic,
return ret;
 }
 
+static int __smp_call_function(void (*func)(void *info), void *info,
+  int nonatomic, int wait)
+{
+   return smp_call_function_map(func,info,nonatomic,wait,cpu_online_map);
+}
+
 int smp_call_function(void (*func) (void *info), void *info, int nonatomic,
int wait)
 {
-   return smp_call_function_map(func,info,nonatomic,wait,cpu_online_map);
+   /* Can deadlock when called with interrupts disabled */
+   WARN_ON(irqs_disabled());
+
+   return __smp_call_function(func, info, nonatomic, wait);
 }
 EXPORT_SYMBOL(smp_call_function);
 
@@ -283,6 +284,9 @@ int smp_call_function_single(int cpu, void (*func) (void 
*info), void *info, int
cpumask_t map = CPU_MASK_NONE;
int ret = 0;
 
+   /* Can deadlock when called with interrupts disabled */
+   WARN_ON(irqs_disabled());
+
if (!cpu_online(cpu))
return -EINVAL;
 
@@ -299,6 +303,11 @@ int smp_call_function_single(int cpu, void (*func) (void 
*info), void *info, int
 }
 EXPORT_SYMBOL(smp_call_function_single);
 
+void smp_send_stop(void)
+{
+   __smp_call_function(stop_this_cpu, NULL, 1, 0);
+}
+
 void smp_call_function_interrupt(void)
 {
void (*func) (void *info);
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: 2.6.23-rc4-mm1 OOPS in forcedeth?

[Satyam Sharma]

On Mon, 17 Sep 2007, Denis V. Lunev wrote:
 Dhaval Giani wrote:
  On Thu, Sep 13, 2007 at 11:51:33PM -0400, Andrew James Wade wrote:

  EIP: [c037d81b] tcp_rto_min+0xb/0x15 SS:ESP 0068:c0596dec

As Vlad Yasevich mentioned, this one is already fixed in 23-rc6.

The forcedeth oops is unrelated, but multiple people have reported that
same oops now -- adding Manfred Spraul to CC. [ original thread is at:
http://lkml.org/lkml/2007/9/1/115 ]
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] powerpc: Avoid pointless WARN_ON(irqs_disabled()) from panic codepath

[Satyam Sharma]

On Tue, 18 Sep 2007, Satyam Sharma wrote:
 
  [ cut here ]
  Badness at arch/powerpc/kernel/smp.c:202
 
 comes when smp_call_function_map() has been called with irqs disabled,
 which is illegal. However, there is a special case, the panic() codepath,
 when we do not want to warn about this -- warning at that time is pointless
 anyway, and only serves to scroll away the *real* cause of the panic and
 distracts from the real bug.

BTW arch/ppc/ has same issue, but that's gonna be removed by next year
anyways, so I think there's no point making a patch for that (?)
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: iunique() fails to return ino_t (after commit 866b04fccbf125cd)

[Satyam Sharma]

On 9/17/07, Jeff Layton <[EMAIL PROTECTED]> wrote:
> On Mon, 17 Sep 2007 00:58:54 +0530
> "Satyam Sharma" <[EMAIL PROTECTED]> wrote:
>
> > Hi Jeff,
> >
> > I think commit 866b04fccbf125cd39f2bdbcfeaa611d39a061a8 was wrong, and
> > introduced a regression.
> >
> > The "relevant" changelog [*] of that patch says:
> >
> >
> > >  on filesystems w/o permanent inode numbers, i_ino values can be larger
> > >  than 32 bits, which can cause problems for some 32 bit userspace programs
> > >  on a 64 bit kernel.  We can't do anything for filesystems that have
> > >  actual >32-bit inode numbers, but on filesystems that generate i_ino
> > >  values on the fly, we should try to have them fit in 32 bits.  We could
> > >  trivially fix this by making the static counters in new_inode and iunique
> > >  32 bits [...]
> > >
> > > [...]
> > > When a 32-bit program that was not compiled with large file offsets does a
> > > stat and gets a st_ino value back that won't fit in the 32 bit field, 
> > > glibc
> > > (correctly) generates an EOVERFLOW error.  We can't do anything about fs's
> > > with larger permanent inode numbers, but when we generate them on the fly,
> > > we ought to try and have them fit within a 32 bit field.
> > >
> > > This patch takes the first step toward this by making the static counters
> > > in these two functions be 32 bits.
> >
> >
> > 1. First and foremost, there was nothing wrong with the existing code that
> >needed to be "fixed" at all, i.e. there was no "problem" to be solved in
> >the first place. As was said, glibc *correctly* returns EOVERFLOW when a
> >userspace application built *without* _FILE_OFFSET_BITS == 64 (i.e.
> >ino_t != __ino64_t) tries to stat(2) a file whose serial number does not
> >fit in the "st_ino" member of struct stat. This behaviour is (1) correct,
> >(2) explicitly mandated by the Single UNIX Specification, and, (3) all
> >userspace programs *must* be prepared to deal with it. [ Should probably
> >mention here that other implementations, such as Solaris, do conform with
> >SUS here. ]
> >
>
> The ideal situation is that everyone would recompile their programs
> with LFS defines. Unfortunately people have old userspace programs to
> which they don't have sources, or that can't easily be recompiled this way.
> These programs would occasionally break when run on 64 bit kernels
> for the reasons I've described.

That's a bad excuse! Such userspace programs are buggy, period.
Let's fix *them*. And, seriously, are you *really* talking of "supporting"
userspace programs whose even *sources* are no longer available ?

The standard is clear and simple -- calls from userspace programs (that
don't define LFS) to stat(2) on a file whose serial number is >2**32 must
see EOVERFLOW. This is *not* a kernel problem that needs "fixing".

Moreover, changing a kernel function such as iunique() (which was expressly
*written* to be used by filesystems to assign ino_t to inodes) to return only
values <= 2**32 to satisfy such buggy programs is just ... bad.

BTW, you might've as well changed the type of "res" in iunique() in that
patch to "unsigned int" too. What's the point of declaring it "ino_t" if we
never assign it any value in the (ino_t - unsigned int) set in the first place?


> > 2. The patch has nothing to do with "32-bit userspace on 64-bit kernels" or
> >compatibility modes whatsoever. It is unrelated and tangential that this
> >behaviour is easy to reproduce on the above mentioned setup. Simply put,
> >the issue is that a userspace program built *without* LFS tried to
> >stat(2) a file with serial number > 2**32. Needless to say, this issue
> >must be solved in the userspace program itself (either by (1) defining
> >LFS, or, (2) making it aware of EOVERFLOW), and not in the kernel.
> >
>
> It most certainly does have something to do with 32 bit userspace on 64
> bit kernels.

No, it doesn't ...

> On a 32 bit kernel, new_inode and iunique generate no
> inode numbers >32 bits. On a 64 bit kernel, they do.

This is *unrelated*. It's completely immaterial how an inode managed to
get a serial number > 2**32. Whether it used iunique() running on a 64-bit
kernel, or it's own little implementation, or whatever. The *real* issue is with
the *userspace program* and not the kernel ... that's the whole point.

[ Also, you're assuming sizeof(long) == sizeof(int) for 32-bit kernels
  here, but okay, probably that'

Re: [PATCH] modpost: detect unterminated device id lists

[Satyam Sharma]

Hi Kees,


On 9/13/07, Kees Cook <[EMAIL PROTECTED]> wrote:
>
> This patch against 2.6.23-rc6 will cause modpost to fail if any device
> id lists are incorrectly terminated, after reporting the offender.
>
> Signed-off-by: Kees Cook <[EMAIL PROTECTED]>

Nice! :-)

BTW a very similar idea (but for a different problem) was discussed in:
http://lkml.org/lkml/2007/8/23/48

I tried doing something about that, but gave up in between. For the
device_id tables, a lot of infrastructure/code already exists in modpost,
but no such luck for kobjects :-( Still, if you can do something about
that, as he mentioned, I bet Greg would gladly accept such a patch :-)


> --- linux-2.6.23-rc6~/scripts/mod/file2alias.c  2007-09-11 23:17:49.0 
> -0700
> +++ linux-2.6.23-rc6/scripts/mod/file2alias.c   2007-09-12 17:41:30.0 
> -0700
> @@ -55,10 +55,13 @@ do {
>   * Check that sizeof(device_id type) are consistent with size of section
>   * in .o file. If in-consistent then userspace and kernel does not agree
>   * on actual size which is a bug.
> + * Also verify that the final entry in the table is all zeros.
>   **/
> -static void device_id_size_check(const char *modname, const char *device_id,
> -unsigned long size, unsigned long id_size)
> +static void device_id_check(const char *modname, const char *device_id,
> +   unsigned long size, unsigned long id_size,
> +   void *symval)

If you pass the Elf_Sym *sym all the way from handle_moddevtable() (which
means you can get rid of the sym->st_size argument in the call chain), then
it would be possible to print out the *symbol name* too here ...


>  {
> +   int i;

uint8_t *p;


> if (size % id_size || size < id_size) {
> fatal("%s: sizeof(struct %s_device_id)=%lu is not a modulo "
>   "of the size of section __mod_%s_device_table=%lu.\n"
> @@ -66,6 +69,18 @@ static void device_id_size_check(const c
>   "in mod_devicetable.h\n",
>   modname, device_id, id_size, device_id, size, 
> device_id);
> }

> +   /* Verify last one is a terminator */
> +   for (i = 0; i < id_size; i++ ) {
> +   if ( *(uint8_t*)(symval+size-id_size+i) ) {

... and:

for (p = symval+size-id_size; p < symval+size; p++) {
if (*p) {

is probably clearer ?


> +   fprintf(stderr,"%s: struct %s_device_id is %lu bytes. 
>  The last of
> %lu is:\n", modname, device_id, id_size, size / id_size);

As I just said, printing out just the modname and device_id "type" sounds
insufficient here. Note that they were sufficient before your patch, because
previously, this function only checked if the device_id *type* itself was
incorrectly defined. But here we're talking about a specific errant *symbol*.


> +   for (i = 0; i < id_size; i++ ) {
> +   fprintf(stderr,"0x%02x ", 
> *(uint8_t*)(symval+size-id_size+i) );
> +   }

Again, "for (p = symval+size-id_size; p < symval+size; p++) {"
and then "fprintf(..., *p);" would be cleaner.


> +   fprintf(stderr,"\n");
> +   fatal("%s: struct %s_device_id is not terminated "
> +   "with a NULL entry!\n", modname, device_id);

Subtle nit, but it's not really a "NULL" entry. It's an "empty object" entry,
not a "NULL" pointer ... how about replacing "a NULL" with "an empty" ?


> +   }
> +   }
>  }
>
>  /* USB is special because the bcdDevice can be matched against a numeric 
> range */
> @@ -168,7 +183,7 @@ static void do_usb_table(void *symval, u
> unsigned int i;
> const unsigned long id_size = sizeof(struct usb_device_id);
>
> -   device_id_size_check(mod->name, "usb", size, id_size);
> +   device_id_check(mod->name, "usb", size, id_size, symval);
>
> /* Leave last one: it's the terminator. */
> size -= id_size;
> @@ -505,7 +520,7 @@ static void do_table(void *symval, unsig
> char alias[500];
> int (*do_entry)(const char *, void *entry, char *alias) = function;
>
> -   device_id_size_check(mod->name, device_id, size, id_size);
> +   device_id_check(mod->name, device_id, size, id_size, symval);
> /* Leave last one: it's the terminator. */
> size -= id_size;
>
> @@ -527,14 +542,22 @@ void handle_moddevtable(struct module *m
> Elf_Sym *sym, const char *symname)
>  {
> void *symval;
> +   char *zeros = NULL;
>
> /* We're looking for a section relative symbol */
> if (!sym->st_shndx || sym->st_shndx >= info->hdr->e_shnum)
> return;
>
> -   symval = (void *)info->hdr
> -   + info->sechdrs[sym->st_shndx].sh_offset
> -   + sym->st_value;
> +   /* Handle all-NULL symbols allocated into .bss */
> +   if 

Re: [PATCH] modpost: detect unterminated device id lists

[Satyam Sharma]

On 9/17/07, Andrew Morton <[EMAIL PROTECTED]> wrote:
> On Wed, 12 Sep 2007 17:49:37 -0700 Kees Cook <[EMAIL PROTECTED]> wrote:
>
> > This patch against 2.6.23-rc6 will cause modpost to fail if any device
> > id lists are incorrectly terminated, after reporting the offender.
>
> I'm getting this:
>
> rusb2/pvrusb2: struct usb_device_id is 20 bytes.  The last of 3 is:
> 0x03 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
> 0x00 0x00 0x00 0x00 0x00
> FATAL: drivers/media/video/pvrusb2/pvrusb2: struct usb_device_id is not 
> terminated
> with a NULL entry!
>
> ("rusb2/pvrusb2" ??)

Hmm? Are you sure you didn't see any "drivers/media/video/pv" before the
"rusb2/pvrusb2" bit? Looking at Kees' patch (and the existing code), I've no
clue how/why this should happen ... will try to reproduce here ...


> but:
>
> struct usb_device_id pvr2_device_table[] = {
> [PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
> [PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
> { USB_DEVICE(0, 0) },
> };
>
> looks OK?
>
> Using plain old "{ }" shut the warning up.

USB_DEVICE(0, 0) is not empty termination, actually, and this looks like
a genuine bug caught by the patch. As that dump shows, USB_DEVICE(0, 0)
assigns "0x03 0x00" (in little endian) to usb_device_id.match_flags. And
I don't think the USB code treats such an entry as an empty entry (?)

Interestingly, the "USB_DEVICE(0, 0)" thing is absent from latest -git
tree and also in my copy of 23-rc4-mm1 -- so this looks like something
you must've merged recently.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

iunique() fails to return ino_t (after commit 866b04fccbf125cd)

[Satyam Sharma]

Hi Jeff,

I think commit 866b04fccbf125cd39f2bdbcfeaa611d39a061a8 was wrong, and
introduced a regression.

The "relevant" changelog [*] of that patch says:


>  on filesystems w/o permanent inode numbers, i_ino values can be larger
>  than 32 bits, which can cause problems for some 32 bit userspace programs
>  on a 64 bit kernel.  We can't do anything for filesystems that have
>  actual >32-bit inode numbers, but on filesystems that generate i_ino
>  values on the fly, we should try to have them fit in 32 bits.  We could
>  trivially fix this by making the static counters in new_inode and iunique
>  32 bits [...]
>
> [...]
> When a 32-bit program that was not compiled with large file offsets does a
> stat and gets a st_ino value back that won't fit in the 32 bit field, glibc
> (correctly) generates an EOVERFLOW error.  We can't do anything about fs's
> with larger permanent inode numbers, but when we generate them on the fly,
> we ought to try and have them fit within a 32 bit field.
>
> This patch takes the first step toward this by making the static counters
> in these two functions be 32 bits.


1. First and foremost, there was nothing wrong with the existing code that
   needed to be "fixed" at all, i.e. there was no "problem" to be solved in
   the first place. As was said, glibc *correctly* returns EOVERFLOW when a
   userspace application built *without* _FILE_OFFSET_BITS == 64 (i.e.
   ino_t != __ino64_t) tries to stat(2) a file whose serial number does not
   fit in the "st_ino" member of struct stat. This behaviour is (1) correct,
   (2) explicitly mandated by the Single UNIX Specification, and, (3) all
   userspace programs *must* be prepared to deal with it. [ Should probably
   mention here that other implementations, such as Solaris, do conform with
   SUS here. ]

2. The patch has nothing to do with "32-bit userspace on 64-bit kernels" or
   compatibility modes whatsoever. It is unrelated and tangential that this
   behaviour is easy to reproduce on the above mentioned setup. Simply put,
   the issue is that a userspace program built *without* LFS tried to
   stat(2) a file with serial number > 2**32. Needless to say, this issue
   must be solved in the userspace program itself (either by (1) defining
   LFS, or, (2) making it aware of EOVERFLOW), and not in the kernel.

3. Solving a problem at a place where it does not exist naturally leads to
   other breakage. After 866b04fccbf125cd, iunique() no longer returns an
   ino_t, but only values <= 2**32, which limits the number of inodes on
   all filesystems that use that function. Needless to say, this is a
   *regression* w.r.t. previous kernels before that commit went in.

4. Last but not the least, the sample testcase program that was discussed
   on LKML last year to show this "problem" was buggy and wrong. A program
   built without LFS will also suffer EOVERFLOW when stat(2)'ing a file
   due to other reasons, such as filesize not fitting inside the "st_size"
   member. Do we propose to "fix" that "problem" in the kernel too ?
   Of course not!


IMHO it's bad to change the kernel's behaviour to avoid buggy userspace
programs from seeing standard-mandated errors being returned from stat(2).

So please reconsider that patch -- IMHO it clearly wasn't correct.


Satyam


[*] BTW, the changelog/patch description of this commit demonstrates
why it is a Bad Thing (tm) to have lengthy [PATCH 0/x] kind of mails
(containing important technical details) preceding a patchset.

I can only guess as to what happened, but reading the archives of the
original submission of this patchset on LKML, I think Andrew had to
append the contents of the [0/3] mail to the git commit of the [1/3]
patch (so as to not lose all those details and ensure that they got saved
in the git history), with the result that most of the changelog of commit
866b04fccbf1 has nothing to do with that particular patch at all, but
instead with other commits, that do not even touch that same file (!)

So guys, please keep "[0/x]" mails short and only as a non-technical
introduction of the patchset. All relevant discussion must come in the
other mails that contain the *real* patches.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: PROBLEM: kernel hang in ohci init

[Satyam Sharma]

Ok, opened up: http://bugzilla.kernel.org/show_bug.cgi?id=9026
and brought it up to date with the discussion and David's comments on this
thread. Timo, please feel free to revisit this later and update us when you find
the time to do so.

[ BTW I think the "add CC:" thing in bugzilla is broken, I was simply unable to
  add David Brownell, linux-acpi@ and Timo to the CC: for that bug, if somebody
  knows how to do this, please add them ... ]

Thanks,

Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: PROBLEM: kernel hang in ohci init

[Satyam Sharma]

On 9/16/07, Satyam Sharma <[EMAIL PROTECTED]> wrote:
> Hi Timo,
>
>
> On 7/15/07, Timo Lindemann <[EMAIL PROTECTED]> wrote:
> > To sum this up:
> >
> > the userspace 2.6.20.6 (the "good" kernel) and 2.6.22 (the "bad" kernel)
> > were compiled in is exactly the same setup. I recompiled "good" to check
> > for that, earlier, but "good" also works then.
> >
> > "good" does not exhibit the printks I placed in the section (the same
> > ones I did for "bad"), making it plausible that the section is not
> > executed at all.
> >
> > dmesg is not captured to disk, netconsole and serial console also do not
> > work (they both did in the "good" kernel). Also, my keyboard does not
> > work with "bad" during that phase -- Magic SysRq is also not working then.
> >
> > I can try to hook up the laptop to an external monitor to capture some
> > more dmesg, and just shoot a photo, but I am right now trying to work
> > with git, as Satyam suggested.
>
> Any updates on this for us? Or did the kernel start booting magically again
> ca. 2.6.23-rc6? ;-)

Should again add that best would still be to simply git-bisect Linus' (mainline)
kernel tree between 2.6.20 (not 2.6.20.6) and 2.6.22 and just find the commit
after which your box stops booting ...


> Anyway, it appears the bug got introduced sometime between 2.6.20 and
> 2.6.22 so probably bugzilla becomes a better place to track this one. Could
> you open up a bug report (similar to your original post) there?
>
> Thanks,
>
> Satyam
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: PROBLEM: kernel hang in ohci init

[Satyam Sharma]

Hi Timo,


On 7/15/07, Timo Lindemann <[EMAIL PROTECTED]> wrote:
> To sum this up:
>
> the userspace 2.6.20.6 (the "good" kernel) and 2.6.22 (the "bad" kernel)
> were compiled in is exactly the same setup. I recompiled "good" to check
> for that, earlier, but "good" also works then.
>
> "good" does not exhibit the printks I placed in the section (the same
> ones I did for "bad"), making it plausible that the section is not
> executed at all.
>
> dmesg is not captured to disk, netconsole and serial console also do not
> work (they both did in the "good" kernel). Also, my keyboard does not
> work with "bad" during that phase -- Magic SysRq is also not working then.
>
> I can try to hook up the laptop to an external monitor to capture some
> more dmesg, and just shoot a photo, but I am right now trying to work
> with git, as Satyam suggested.

Any updates on this for us? Or did the kernel start booting magically again
ca. 2.6.23-rc6? ;-)

Anyway, it appears the bug got introduced sometime between 2.6.20 and
2.6.22 so probably bugzilla becomes a better place to track this one. Could
you open up a bug report (similar to your original post) there?

Thanks,

Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: PROBLEM: kernel hang in ohci init

[Satyam Sharma]

Hi Timo,


On 7/15/07, Timo Lindemann [EMAIL PROTECTED] wrote:
 To sum this up:

 the userspace 2.6.20.6 (the good kernel) and 2.6.22 (the bad kernel)
 were compiled in is exactly the same setup. I recompiled good to check
 for that, earlier, but good also works then.

 good does not exhibit the printks I placed in the section (the same
 ones I did for bad), making it plausible that the section is not
 executed at all.

 dmesg is not captured to disk, netconsole and serial console also do not
 work (they both did in the good kernel). Also, my keyboard does not
 work with bad during that phase -- Magic SysRq is also not working then.

 I can try to hook up the laptop to an external monitor to capture some
 more dmesg, and just shoot a photo, but I am right now trying to work
 with git, as Satyam suggested.

Any updates on this for us? Or did the kernel start booting magically again
ca. 2.6.23-rc6? ;-)

Anyway, it appears the bug got introduced sometime between 2.6.20 and
2.6.22 so probably bugzilla becomes a better place to track this one. Could
you open up a bug report (similar to your original post) there?

Thanks,

Satyam
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: PROBLEM: kernel hang in ohci init

[Satyam Sharma]

On 9/16/07, Satyam Sharma [EMAIL PROTECTED] wrote:
 Hi Timo,


 On 7/15/07, Timo Lindemann [EMAIL PROTECTED] wrote:
  To sum this up:
 
  the userspace 2.6.20.6 (the good kernel) and 2.6.22 (the bad kernel)
  were compiled in is exactly the same setup. I recompiled good to check
  for that, earlier, but good also works then.
 
  good does not exhibit the printks I placed in the section (the same
  ones I did for bad), making it plausible that the section is not
  executed at all.
 
  dmesg is not captured to disk, netconsole and serial console also do not
  work (they both did in the good kernel). Also, my keyboard does not
  work with bad during that phase -- Magic SysRq is also not working then.
 
  I can try to hook up the laptop to an external monitor to capture some
  more dmesg, and just shoot a photo, but I am right now trying to work
  with git, as Satyam suggested.

 Any updates on this for us? Or did the kernel start booting magically again
 ca. 2.6.23-rc6? ;-)

Should again add that best would still be to simply git-bisect Linus' (mainline)
kernel tree between 2.6.20 (not 2.6.20.6) and 2.6.22 and just find the commit
after which your box stops booting ...


 Anyway, it appears the bug got introduced sometime between 2.6.20 and
 2.6.22 so probably bugzilla becomes a better place to track this one. Could
 you open up a bug report (similar to your original post) there?

 Thanks,

 Satyam

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: PROBLEM: kernel hang in ohci init

[Satyam Sharma]

Ok, opened up: http://bugzilla.kernel.org/show_bug.cgi?id=9026
and brought it up to date with the discussion and David's comments on this
thread. Timo, please feel free to revisit this later and update us when you find
the time to do so.

[ BTW I think the add CC: thing in bugzilla is broken, I was simply unable to
  add David Brownell, linux-acpi@ and Timo to the CC: for that bug, if somebody
  knows how to do this, please add them ... ]

Thanks,

Satyam
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

iunique() fails to return ino_t (after commit 866b04fccbf125cd)

[Satyam Sharma]

Hi Jeff,

I think commit 866b04fccbf125cd39f2bdbcfeaa611d39a061a8 was wrong, and
introduced a regression.

The relevant changelog [*] of that patch says:


  on filesystems w/o permanent inode numbers, i_ino values can be larger
  than 32 bits, which can cause problems for some 32 bit userspace programs
  on a 64 bit kernel.  We can't do anything for filesystems that have
  actual 32-bit inode numbers, but on filesystems that generate i_ino
  values on the fly, we should try to have them fit in 32 bits.  We could
  trivially fix this by making the static counters in new_inode and iunique
  32 bits [...]

 [...]
 When a 32-bit program that was not compiled with large file offsets does a
 stat and gets a st_ino value back that won't fit in the 32 bit field, glibc
 (correctly) generates an EOVERFLOW error.  We can't do anything about fs's
 with larger permanent inode numbers, but when we generate them on the fly,
 we ought to try and have them fit within a 32 bit field.

 This patch takes the first step toward this by making the static counters
 in these two functions be 32 bits.


1. First and foremost, there was nothing wrong with the existing code that
   needed to be fixed at all, i.e. there was no problem to be solved in
   the first place. As was said, glibc *correctly* returns EOVERFLOW when a
   userspace application built *without* _FILE_OFFSET_BITS == 64 (i.e.
   ino_t != __ino64_t) tries to stat(2) a file whose serial number does not
   fit in the st_ino member of struct stat. This behaviour is (1) correct,
   (2) explicitly mandated by the Single UNIX Specification, and, (3) all
   userspace programs *must* be prepared to deal with it. [ Should probably
   mention here that other implementations, such as Solaris, do conform with
   SUS here. ]

2. The patch has nothing to do with 32-bit userspace on 64-bit kernels or
   compatibility modes whatsoever. It is unrelated and tangential that this
   behaviour is easy to reproduce on the above mentioned setup. Simply put,
   the issue is that a userspace program built *without* LFS tried to
   stat(2) a file with serial number  2**32. Needless to say, this issue
   must be solved in the userspace program itself (either by (1) defining
   LFS, or, (2) making it aware of EOVERFLOW), and not in the kernel.

3. Solving a problem at a place where it does not exist naturally leads to
   other breakage. After 866b04fccbf125cd, iunique() no longer returns an
   ino_t, but only values = 2**32, which limits the number of inodes on
   all filesystems that use that function. Needless to say, this is a
   *regression* w.r.t. previous kernels before that commit went in.

4. Last but not the least, the sample testcase program that was discussed
   on LKML last year to show this problem was buggy and wrong. A program
   built without LFS will also suffer EOVERFLOW when stat(2)'ing a file
   due to other reasons, such as filesize not fitting inside the st_size
   member. Do we propose to fix that problem in the kernel too ?
   Of course not!


IMHO it's bad to change the kernel's behaviour to avoid buggy userspace
programs from seeing standard-mandated errors being returned from stat(2).

So please reconsider that patch -- IMHO it clearly wasn't correct.


Satyam


[*] BTW, the changelog/patch description of this commit demonstrates
why it is a Bad Thing (tm) to have lengthy [PATCH 0/x] kind of mails
(containing important technical details) preceding a patchset.

I can only guess as to what happened, but reading the archives of the
original submission of this patchset on LKML, I think Andrew had to
append the contents of the [0/3] mail to the git commit of the [1/3]
patch (so as to not lose all those details and ensure that they got saved
in the git history), with the result that most of the changelog of commit
866b04fccbf1 has nothing to do with that particular patch at all, but
instead with other commits, that do not even touch that same file (!)

So guys, please keep [0/x] mails short and only as a non-technical
introduction of the patchset. All relevant discussion must come in the
other mails that contain the *real* patches.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] modpost: detect unterminated device id lists

[Satyam Sharma]

On 9/17/07, Andrew Morton [EMAIL PROTECTED] wrote:
 On Wed, 12 Sep 2007 17:49:37 -0700 Kees Cook [EMAIL PROTECTED] wrote:

  This patch against 2.6.23-rc6 will cause modpost to fail if any device
  id lists are incorrectly terminated, after reporting the offender.

 I'm getting this:

 rusb2/pvrusb2: struct usb_device_id is 20 bytes.  The last of 3 is:
 0x03 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
 0x00 0x00 0x00 0x00 0x00
 FATAL: drivers/media/video/pvrusb2/pvrusb2: struct usb_device_id is not 
 terminated
 with a NULL entry!

 (rusb2/pvrusb2 ??)

Hmm? Are you sure you didn't see any drivers/media/video/pv before the
rusb2/pvrusb2 bit? Looking at Kees' patch (and the existing code), I've no
clue how/why this should happen ... will try to reproduce here ...


 but:

 struct usb_device_id pvr2_device_table[] = {
 [PVR2_HDW_TYPE_29XXX] = { USB_DEVICE(0x2040, 0x2900) },
 [PVR2_HDW_TYPE_24XXX] = { USB_DEVICE(0x2040, 0x2400) },
 { USB_DEVICE(0, 0) },
 };

 looks OK?

 Using plain old { } shut the warning up.

USB_DEVICE(0, 0) is not empty termination, actually, and this looks like
a genuine bug caught by the patch. As that dump shows, USB_DEVICE(0, 0)
assigns 0x03 0x00 (in little endian) to usb_device_id.match_flags. And
I don't think the USB code treats such an entry as an empty entry (?)

Interestingly, the USB_DEVICE(0, 0) thing is absent from latest -git
tree and also in my copy of 23-rc4-mm1 -- so this looks like something
you must've merged recently.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] modpost: detect unterminated device id lists

[Satyam Sharma]

Hi Kees,


On 9/13/07, Kees Cook [EMAIL PROTECTED] wrote:

 This patch against 2.6.23-rc6 will cause modpost to fail if any device
 id lists are incorrectly terminated, after reporting the offender.

 Signed-off-by: Kees Cook [EMAIL PROTECTED]

Nice! :-)

BTW a very similar idea (but for a different problem) was discussed in:
http://lkml.org/lkml/2007/8/23/48

I tried doing something about that, but gave up in between. For the
device_id tables, a lot of infrastructure/code already exists in modpost,
but no such luck for kobjects :-( Still, if you can do something about
that, as he mentioned, I bet Greg would gladly accept such a patch :-)


 --- linux-2.6.23-rc6~/scripts/mod/file2alias.c  2007-09-11 23:17:49.0 
 -0700
 +++ linux-2.6.23-rc6/scripts/mod/file2alias.c   2007-09-12 17:41:30.0 
 -0700
 @@ -55,10 +55,13 @@ do {
   * Check that sizeof(device_id type) are consistent with size of section
   * in .o file. If in-consistent then userspace and kernel does not agree
   * on actual size which is a bug.
 + * Also verify that the final entry in the table is all zeros.
   **/
 -static void device_id_size_check(const char *modname, const char *device_id,
 -unsigned long size, unsigned long id_size)
 +static void device_id_check(const char *modname, const char *device_id,
 +   unsigned long size, unsigned long id_size,
 +   void *symval)

If you pass the Elf_Sym *sym all the way from handle_moddevtable() (which
means you can get rid of the sym-st_size argument in the call chain), then
it would be possible to print out the *symbol name* too here ...


  {
 +   int i;

uint8_t *p;


 if (size % id_size || size  id_size) {
 fatal(%s: sizeof(struct %s_device_id)=%lu is not a modulo 
   of the size of section __mod_%s_device_table=%lu.\n
 @@ -66,6 +69,18 @@ static void device_id_size_check(const c
   in mod_devicetable.h\n,
   modname, device_id, id_size, device_id, size, 
 device_id);
 }

 +   /* Verify last one is a terminator */
 +   for (i = 0; i  id_size; i++ ) {
 +   if ( *(uint8_t*)(symval+size-id_size+i) ) {

... and:

for (p = symval+size-id_size; p  symval+size; p++) {
if (*p) {

is probably clearer ?


 +   fprintf(stderr,%s: struct %s_device_id is %lu bytes. 
  The last of
 %lu is:\n, modname, device_id, id_size, size / id_size);

As I just said, printing out just the modname and device_id type sounds
insufficient here. Note that they were sufficient before your patch, because
previously, this function only checked if the device_id *type* itself was
incorrectly defined. But here we're talking about a specific errant *symbol*.


 +   for (i = 0; i  id_size; i++ ) {
 +   fprintf(stderr,0x%02x , 
 *(uint8_t*)(symval+size-id_size+i) );
 +   }

Again, for (p = symval+size-id_size; p  symval+size; p++) {
and then fprintf(..., *p); would be cleaner.


 +   fprintf(stderr,\n);
 +   fatal(%s: struct %s_device_id is not terminated 
 +   with a NULL entry!\n, modname, device_id);

Subtle nit, but it's not really a NULL entry. It's an empty object entry,
not a NULL pointer ... how about replacing a NULL with an empty ?


 +   }
 +   }
  }

  /* USB is special because the bcdDevice can be matched against a numeric 
 range */
 @@ -168,7 +183,7 @@ static void do_usb_table(void *symval, u
 unsigned int i;
 const unsigned long id_size = sizeof(struct usb_device_id);

 -   device_id_size_check(mod-name, usb, size, id_size);
 +   device_id_check(mod-name, usb, size, id_size, symval);

 /* Leave last one: it's the terminator. */
 size -= id_size;
 @@ -505,7 +520,7 @@ static void do_table(void *symval, unsig
 char alias[500];
 int (*do_entry)(const char *, void *entry, char *alias) = function;

 -   device_id_size_check(mod-name, device_id, size, id_size);
 +   device_id_check(mod-name, device_id, size, id_size, symval);
 /* Leave last one: it's the terminator. */
 size -= id_size;

 @@ -527,14 +542,22 @@ void handle_moddevtable(struct module *m
 Elf_Sym *sym, const char *symname)
  {
 void *symval;
 +   char *zeros = NULL;

 /* We're looking for a section relative symbol */
 if (!sym-st_shndx || sym-st_shndx = info-hdr-e_shnum)
 return;

 -   symval = (void *)info-hdr
 -   + info-sechdrs[sym-st_shndx].sh_offset
 -   + sym-st_value;
 +   /* Handle all-NULL symbols allocated into .bss */
 +   if (info-sechdrs[sym-st_shndx].sh_type  SHT_NOBITS) {
 +   zeros = calloc(1, sym-st_size);
 +   symval = zeros;
 +   }

Hmm, 

Re: iunique() fails to return ino_t (after commit 866b04fccbf125cd)

[Satyam Sharma]

On 9/17/07, Jeff Layton [EMAIL PROTECTED] wrote:
 On Mon, 17 Sep 2007 00:58:54 +0530
 Satyam Sharma [EMAIL PROTECTED] wrote:

  Hi Jeff,
 
  I think commit 866b04fccbf125cd39f2bdbcfeaa611d39a061a8 was wrong, and
  introduced a regression.
 
  The relevant changelog [*] of that patch says:
 
 
on filesystems w/o permanent inode numbers, i_ino values can be larger
than 32 bits, which can cause problems for some 32 bit userspace programs
on a 64 bit kernel.  We can't do anything for filesystems that have
actual 32-bit inode numbers, but on filesystems that generate i_ino
values on the fly, we should try to have them fit in 32 bits.  We could
trivially fix this by making the static counters in new_inode and iunique
32 bits [...]
  
   [...]
   When a 32-bit program that was not compiled with large file offsets does a
   stat and gets a st_ino value back that won't fit in the 32 bit field, 
   glibc
   (correctly) generates an EOVERFLOW error.  We can't do anything about fs's
   with larger permanent inode numbers, but when we generate them on the fly,
   we ought to try and have them fit within a 32 bit field.
  
   This patch takes the first step toward this by making the static counters
   in these two functions be 32 bits.
 
 
  1. First and foremost, there was nothing wrong with the existing code that
 needed to be fixed at all, i.e. there was no problem to be solved in
 the first place. As was said, glibc *correctly* returns EOVERFLOW when a
 userspace application built *without* _FILE_OFFSET_BITS == 64 (i.e.
 ino_t != __ino64_t) tries to stat(2) a file whose serial number does not
 fit in the st_ino member of struct stat. This behaviour is (1) correct,
 (2) explicitly mandated by the Single UNIX Specification, and, (3) all
 userspace programs *must* be prepared to deal with it. [ Should probably
 mention here that other implementations, such as Solaris, do conform with
 SUS here. ]
 

 The ideal situation is that everyone would recompile their programs
 with LFS defines. Unfortunately people have old userspace programs to
 which they don't have sources, or that can't easily be recompiled this way.
 These programs would occasionally break when run on 64 bit kernels
 for the reasons I've described.

That's a bad excuse! Such userspace programs are buggy, period.
Let's fix *them*. And, seriously, are you *really* talking of supporting
userspace programs whose even *sources* are no longer available ?

The standard is clear and simple -- calls from userspace programs (that
don't define LFS) to stat(2) on a file whose serial number is 2**32 must
see EOVERFLOW. This is *not* a kernel problem that needs fixing.

Moreover, changing a kernel function such as iunique() (which was expressly
*written* to be used by filesystems to assign ino_t to inodes) to return only
values = 2**32 to satisfy such buggy programs is just ... bad.

BTW, you might've as well changed the type of res in iunique() in that
patch to unsigned int too. What's the point of declaring it ino_t if we
never assign it any value in the (ino_t - unsigned int) set in the first place?


  2. The patch has nothing to do with 32-bit userspace on 64-bit kernels or
 compatibility modes whatsoever. It is unrelated and tangential that this
 behaviour is easy to reproduce on the above mentioned setup. Simply put,
 the issue is that a userspace program built *without* LFS tried to
 stat(2) a file with serial number  2**32. Needless to say, this issue
 must be solved in the userspace program itself (either by (1) defining
 LFS, or, (2) making it aware of EOVERFLOW), and not in the kernel.
 

 It most certainly does have something to do with 32 bit userspace on 64
 bit kernels.

No, it doesn't ...

 On a 32 bit kernel, new_inode and iunique generate no
 inode numbers 32 bits. On a 64 bit kernel, they do.

This is *unrelated*. It's completely immaterial how an inode managed to
get a serial number  2**32. Whether it used iunique() running on a 64-bit
kernel, or it's own little implementation, or whatever. The *real* issue is with
the *userspace program* and not the kernel ... that's the whole point.

[ Also, you're assuming sizeof(long) == sizeof(int) for 32-bit kernels
  here, but okay, probably that's true for all supported targets ... ]


 This means that
 programs that are built this way may eventually fail on a 64 bit kernel
 when the inode counter grows large enough. Those programs will work
 indefinitely on a 32 bit kernel.

See below, for why such programs are buggy ...


  3. Solving a problem at a place where it does not exist naturally leads to
 other breakage. After 866b04fccbf125cd, iunique() no longer returns an
 ino_t, but only values = 2**32, which limits the number of inodes on
 all filesystems that use that function. Needless to say, this is a
 *regression* w.r.t. previous kernels before that commit went in.
 

 Why is this a problem

Re: [Minor patch] Reduce __print_symbol/sprint_symbol stack usage.

[Satyam Sharma]

Hi,


On 9/15/07, Gilboa Davara <[EMAIL PROTECTED]> wrote:
> Hello all,
>
> In a small exchange in fedora-kernel-list [1] Eric Sandeen has pointed
> out a possible stack overflow... when CONFIG_DEBUG_STACKOVERFLOW is
> enabled. (Though not limited to it)

Yeah, I have experienced this phenomenon/problem myself.


> Code path is simple: do_IRQ detects a a near stack overflow condition
> and calls show_trace_log_lvl which, down the line uses __print_symbol
> and sprint_symbol to print the call stack.
> However,  both __print_symbol + sprint_symbol are eating no-less then
> 128+223 bytes on static char arrays, which, given the fact that this
> code path is actually generated by low stack warning (< 512 bytes),
> might turn a minor (?) problem (low stack) into a full blown crash.

__print_symbol() and sprint_symbol() are called multiple times during
oopsen / panics. I think those buffers were static char arrays for a good
reason ...


> The patch itself is fairly simple and non-intrusive. [2]
> Both functions allocate memory for their buffers - falling back to
> minimal address display if memory allocation fails.
>
> P.S. Can anyone please point me to the maintainer of kernel/syms? (I
> rather not spam world + dog for such a minor patch)

Anything that touches the panic codepath is important, not minor at all.


> Gilboa Davara <[EMAIL PROTECTED]>
>
> [1]
> http://www.mail-archive.com/[EMAIL PROTECTED]/msg00640.html
>
> [2]. In theory, there's a second option: pre-allocating memory on a
> per_cpu basis, however:
> A. dump_trace/stack are usually called when something bad has happened -
> reducing the need for performance optimizations.

That's not a performance optimization -- avoiding repeated kmalloc()'s in the
panic codepath sounds like a *requirement* to me.


> B. per_cpu allocation will also require local_irq_disable/enable as both
> functions are being called from multiple contexts. Too much hassle.

I think not bothering about any locking in these codepaths may not be an
entirely unreasonable thing to do (sorry about the triple negation in the
sentence). What I mean is that there are places in these codepaths where
we already don't bother with locking ...

Overall I don't much like introducing kmalloc(GFP_ATOMIC) in these codepaths
and would ask you guys to consider some other pre-allocation (i.e. static
allocation not on stack but in .data) alternative instead ...


Satyam


> --- linux-2.6/kernel/kallsyms.orig  2007-09-15 11:46:54.0 +0300
> +++ linux-2.6/kernel/kallsyms.c 2007-09-15 14:25:21.0 +0300
> @@ -309,30 +309,62 @@ int lookup_symbol_attrs(unsigned long ad
>  /* Look up a kernel symbol and return it in a text buffer. */
>  int sprint_symbol(char *buffer, unsigned long address)
>  {
> -   char *modname;
> -   const char *name;
> unsigned long offset, size;
> -   char namebuf[KSYM_NAME_LEN];
> +   const char *name = NULL;
> +   char *namebuf = NULL;
> +   char *modname;
> +   int ret;
> +
> +
> +   /* Static buffer allocation.
> +  Required in-order to reduce stack footprint on
> +do_IRQ/4KSTACK/i386 */
> +   namebuf = kmalloc(KSYM_NAME_LEN, GFP_ATOMIC);
> +   if (namebuf)
> +   name = kallsyms_lookup(address, , ,
> +   , namebuf);
>
> -   name = kallsyms_lookup(address, , , , namebuf);
> if (!name)
> -   return sprintf(buffer, "0x%lx", address);
> +   ret = sprintf(buffer, "0x%lx", address);
> +   else {
> +   if (modname)
> +   ret = sprintf(buffer, "%s+%#lx/%#lx [%s]",
> +   name, offset, size, modname);
> +   else
> +   ret = sprintf(buffer, "%s+%#lx/%#lx",
> +   name, offset, size);
> +   }
>
> -   if (modname)
> -   return sprintf(buffer, "%s+%#lx/%#lx [%s]", name, offset,
> -   size, modname);
> -   else
> -   return sprintf(buffer, "%s+%#lx/%#lx", name, offset, size);
> +   if (namebuf)
> +   kfree(namebuf);
> +
> +   return ret;
>  }
>
>  /* Look up a kernel symbol and print it to the kernel messages. */
>  void __print_symbol(const char *fmt, unsigned long address)
>  {
> -   char buffer[KSYM_SYMBOL_LEN];
> +   char *buffer = NULL;
>
> -   sprint_symbol(buffer, address);
>
> -   printk(fmt, buffer);
> +   /* Static buffer allocation.
> +  Required in-order to reduce stack footprint on
> +do_IRQ/4KSTACK/i386 */
> +   buffer = kmalloc(KSYM_SYMBOL_LEN, GFP_ATOMIC);
> +   if (buffer) {
> +   sprint_symbol(buffer, address);
> +   printk(fmt, buffer);
> +   kfree(buffer);
> +   } else {
> +   /* Address + '0x' + NULL. */
> +   char sbuffer[(BITS_PER_LONG / 4) + 3];
> +
> +   /* 

Re: [Minor patch] Reduce __print_symbol/sprint_symbol stack usage.

[Satyam Sharma]

Hi,


On 9/15/07, Gilboa Davara [EMAIL PROTECTED] wrote:
 Hello all,

 In a small exchange in fedora-kernel-list [1] Eric Sandeen has pointed
 out a possible stack overflow... when CONFIG_DEBUG_STACKOVERFLOW is
 enabled. (Though not limited to it)

Yeah, I have experienced this phenomenon/problem myself.


 Code path is simple: do_IRQ detects a a near stack overflow condition
 and calls show_trace_log_lvl which, down the line uses __print_symbol
 and sprint_symbol to print the call stack.
 However,  both __print_symbol + sprint_symbol are eating no-less then
 128+223 bytes on static char arrays, which, given the fact that this
 code path is actually generated by low stack warning ( 512 bytes),
 might turn a minor (?) problem (low stack) into a full blown crash.

__print_symbol() and sprint_symbol() are called multiple times during
oopsen / panics. I think those buffers were static char arrays for a good
reason ...


 The patch itself is fairly simple and non-intrusive. [2]
 Both functions allocate memory for their buffers - falling back to
 minimal address display if memory allocation fails.

 P.S. Can anyone please point me to the maintainer of kernel/syms? (I
 rather not spam world + dog for such a minor patch)

Anything that touches the panic codepath is important, not minor at all.


 Gilboa Davara [EMAIL PROTECTED]

 [1]
 http://www.mail-archive.com/[EMAIL PROTECTED]/msg00640.html

 [2]. In theory, there's a second option: pre-allocating memory on a
 per_cpu basis, however:
 A. dump_trace/stack are usually called when something bad has happened -
 reducing the need for performance optimizations.

That's not a performance optimization -- avoiding repeated kmalloc()'s in the
panic codepath sounds like a *requirement* to me.


 B. per_cpu allocation will also require local_irq_disable/enable as both
 functions are being called from multiple contexts. Too much hassle.

I think not bothering about any locking in these codepaths may not be an
entirely unreasonable thing to do (sorry about the triple negation in the
sentence). What I mean is that there are places in these codepaths where
we already don't bother with locking ...

Overall I don't much like introducing kmalloc(GFP_ATOMIC) in these codepaths
and would ask you guys to consider some other pre-allocation (i.e. static
allocation not on stack but in .data) alternative instead ...


Satyam


 --- linux-2.6/kernel/kallsyms.orig  2007-09-15 11:46:54.0 +0300
 +++ linux-2.6/kernel/kallsyms.c 2007-09-15 14:25:21.0 +0300
 @@ -309,30 +309,62 @@ int lookup_symbol_attrs(unsigned long ad
  /* Look up a kernel symbol and return it in a text buffer. */
  int sprint_symbol(char *buffer, unsigned long address)
  {
 -   char *modname;
 -   const char *name;
 unsigned long offset, size;
 -   char namebuf[KSYM_NAME_LEN];
 +   const char *name = NULL;
 +   char *namebuf = NULL;
 +   char *modname;
 +   int ret;
 +
 +
 +   /* Static buffer allocation.
 +  Required in-order to reduce stack footprint on
 +do_IRQ/4KSTACK/i386 */
 +   namebuf = kmalloc(KSYM_NAME_LEN, GFP_ATOMIC);
 +   if (namebuf)
 +   name = kallsyms_lookup(address, size, offset,
 +   modname, namebuf);

 -   name = kallsyms_lookup(address, size, offset, modname, namebuf);
 if (!name)
 -   return sprintf(buffer, 0x%lx, address);
 +   ret = sprintf(buffer, 0x%lx, address);
 +   else {
 +   if (modname)
 +   ret = sprintf(buffer, %s+%#lx/%#lx [%s],
 +   name, offset, size, modname);
 +   else
 +   ret = sprintf(buffer, %s+%#lx/%#lx,
 +   name, offset, size);
 +   }

 -   if (modname)
 -   return sprintf(buffer, %s+%#lx/%#lx [%s], name, offset,
 -   size, modname);
 -   else
 -   return sprintf(buffer, %s+%#lx/%#lx, name, offset, size);
 +   if (namebuf)
 +   kfree(namebuf);
 +
 +   return ret;
  }

  /* Look up a kernel symbol and print it to the kernel messages. */
  void __print_symbol(const char *fmt, unsigned long address)
  {
 -   char buffer[KSYM_SYMBOL_LEN];
 +   char *buffer = NULL;

 -   sprint_symbol(buffer, address);

 -   printk(fmt, buffer);
 +   /* Static buffer allocation.
 +  Required in-order to reduce stack footprint on
 +do_IRQ/4KSTACK/i386 */
 +   buffer = kmalloc(KSYM_SYMBOL_LEN, GFP_ATOMIC);
 +   if (buffer) {
 +   sprint_symbol(buffer, address);
 +   printk(fmt, buffer);
 +   kfree(buffer);
 +   } else {
 +   /* Address + '0x' + NULL. */
 +   char sbuffer[(BITS_PER_LONG / 4) + 3];
 +
 +   /* Fall-back mode.
 +  Memory allocation failed.
 +