* [net-next PATCH 3/5] be2net: fix FW default for VF tx-rate
From: Sathya Perla @ 2012-08-29 6:37 UTC (permalink / raw)
To: netdev; +Cc: Vasundhara Volam, Sathya Perla
In-Reply-To: <1346222264-20819-1-git-send-email-sathya.perla@emulex.com>
From: Vasundhara Volam <vasundhara.volam@emulex.com>
BE3 FW initializes VF tx-rate to 100Mbps. Fix this to 10Gbps.
Signed-off-by: Vasundhara Volam <vasundhara.volam@emulex.com>
Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
---
drivers/net/ethernet/emulex/benet/be_main.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index def2f66..1494f2fc 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -2645,8 +2645,8 @@ static int be_vf_setup(struct be_adapter *adapter)
}
for_all_vfs(adapter, vf_cfg, vf) {
- status = be_cmd_link_status_query(adapter, NULL, &lnk_speed,
- NULL, vf + 1);
+ lnk_speed = 1000;
+ status = be_cmd_set_qos(adapter, lnk_speed, vf + 1);
if (status)
goto err;
vf_cfg->tx_rate = lnk_speed * 10;
--
1.7.4
^ permalink raw reply related
* [net-next PATCH 4/5] be2net: fixup malloc/free of adapter->pmac_id
From: Sathya Perla @ 2012-08-29 6:37 UTC (permalink / raw)
To: netdev; +Cc: Sathya Perla
In-Reply-To: <1346222264-20819-1-git-send-email-sathya.perla@emulex.com>
Free was missing and kcalloc() is better placed in be_ctrl_init()
Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
---
drivers/net/ethernet/emulex/benet/be_main.c | 13 +++++++------
1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index 1494f2fc..6d139d6 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -3438,6 +3438,7 @@ static void be_ctrl_cleanup(struct be_adapter *adapter)
if (mem->va)
dma_free_coherent(&adapter->pdev->dev, mem->size, mem->va,
mem->dma);
+ kfree(adapter->pmac_id);
}
static int be_ctrl_init(struct be_adapter *adapter)
@@ -3474,6 +3475,12 @@ static int be_ctrl_init(struct be_adapter *adapter)
}
memset(rx_filter->va, 0, rx_filter->size);
+ /* primary mac needs 1 pmac entry */
+ adapter->pmac_id = kcalloc(adapter->max_pmac_cnt + 1,
+ sizeof(*adapter->pmac_id), GFP_KERNEL);
+ if (!adapter->pmac_id)
+ return -ENOMEM;
+
mutex_init(&adapter->mbox_lock);
spin_lock_init(&adapter->mcc_lock);
spin_lock_init(&adapter->mcc_cq_lock);
@@ -3610,12 +3617,6 @@ static int be_get_initial_config(struct be_adapter *adapter)
else
adapter->max_pmac_cnt = BE_VF_UC_PMAC_COUNT;
- /* primary mac needs 1 pmac entry */
- adapter->pmac_id = kcalloc(adapter->max_pmac_cnt + 1,
- sizeof(u32), GFP_KERNEL);
- if (!adapter->pmac_id)
- return -ENOMEM;
-
status = be_cmd_get_cntl_attributes(adapter);
if (status)
return status;
--
1.7.4
^ permalink raw reply related
* [net-next PATCH 5/5] be2net: modify log msg for lack of privilege error
From: Sathya Perla @ 2012-08-29 6:37 UTC (permalink / raw)
To: netdev; +Cc: Vasundhara Volam, Sathya Perla
In-Reply-To: <1346222264-20819-1-git-send-email-sathya.perla@emulex.com>
From: Vasundhara Volam <vasundhara.volam@emulex.com>
Signed-off-by: Vasundhara Volam <vasundhara.volam@emulex.com>
Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
---
drivers/net/ethernet/emulex/benet/be_cmds.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/ethernet/emulex/benet/be_cmds.c b/drivers/net/ethernet/emulex/benet/be_cmds.c
index 7fac97b..fc68ca0 100644
--- a/drivers/net/ethernet/emulex/benet/be_cmds.c
+++ b/drivers/net/ethernet/emulex/benet/be_cmds.c
@@ -120,7 +120,7 @@ static int be_mcc_compl_process(struct be_adapter *adapter,
if (compl_status == MCC_STATUS_UNAUTHORIZED_REQUEST) {
dev_warn(&adapter->pdev->dev,
- "opcode %d-%d is not permitted\n",
+ "VF is not privileged to issue opcode %d-%d\n",
opcode, subsystem);
} else {
extd_status = (compl->status >> CQE_STATUS_EXTD_SHIFT) &
--
1.7.4
^ permalink raw reply related
* Re: [PATCH V2 2/2] ipvs: Extend MTU check to account for IPv6 NAT defrag changes
From: Jesper Dangaard Brouer @ 2012-08-29 7:02 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, Patrick McHardy, lvs-devel, Julian Anastasov,
Simon Horman, Pablo Neira Ayuso, Hans Schillstrom, Wensong Zhang,
netfilter-devel
In-Reply-To: <1346165359.3571.9.camel@edumazet-glaptop>
On Tue, 2012-08-28 at 07:49 -0700, Eric Dumazet wrote:
> On Tue, 2012-08-28 at 16:23 +0200, Jesper Dangaard Brouer wrote:
> > This patch is necessary, to make IPVS work, after Patrick McHardys
> > IPv6 NAT defragmentation changes.
> >
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > ---
> > In V2: the tunnel mode is no longer a special case.
> >
> > net/netfilter/ipvs/ip_vs_xmit.c | 9 ++++++++-
> > 1 files changed, 8 insertions(+), 1 deletions(-)
> >
> > diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
> > index 67a3978..56f6d5d 100644
> > --- a/net/netfilter/ipvs/ip_vs_xmit.c
> > +++ b/net/netfilter/ipvs/ip_vs_xmit.c
> > @@ -88,7 +88,14 @@ __ip_vs_dst_check(struct ip_vs_dest *dest, u32 rtos)
> > static inline bool
> > __mtu_check_toobig_v6(const struct sk_buff *skb, u32 mtu)
> > {
> > - if (skb->len > mtu && !skb_is_gso(skb)) {
> > + if (IP6CB(skb)->frag_max_size) {
> > + /* frag_max_size tell us that, this packet have been
> > + * defragmented by netfilter IPv6 conntrack module.
> > + */
> > + if (IP6CB(skb)->frag_max_size > mtu)
> > + return true; /* largest fragment violate MTU */
Implicit: else
return false
(if it makes it more clear, not sure)
> > + }
> > + else if (skb->len > mtu && !skb_is_gso(skb)) {
> > return true; /* Packet size violate MTU size */
> > }
>
> Couldnt you use a single test ?
>
> if (IP6CB(skb)->frag_max_size > mtu)
> return true;
>
> if (skb->len > mtu && !skb_is_gso(skb))
> return true;
>
Nope, this will not work.
If (IP6CB(skb)->frag_max_size > 0) then we have a defragmented packet,
this means that skb->len cannot be used for MTU checking, because
skb->len is now the total length of all the fragments (which your
solution will fall-through to)
The unreadable version of the function is:
static inline bool
__mtu_check_toobig_v6_unreadable(const struct sk_buff *skb, u32 mtu)
{
return !!((!IP6CB(skb)->frag_max_size &&
(skb->len > mtu && !skb_is_gso(skb)))
|| IP6CB(skb)->frag_max_size > mtu);
}
Perhaps you like this version better (you seem to use constructions like
this), but I really dislike it, because I (personally) find it
harder/slower to read this kind of code, and I also believe its more
error prone when someone needs to extend this.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [Qemu-devel] macvlan/macvtap: guest/host cannot communicate when network cable is unplugged
From: Stefan Hajnoczi @ 2012-08-29 7:03 UTC (permalink / raw)
To: ching; +Cc: qemu-devel, kaber, Michael S. Tsirkin, netdev
In-Reply-To: <503D486B.1030208@gmail.com>
On Tue, Aug 28, 2012 at 11:38 PM, ching <lsching17@gmail.com> wrote:
> Hi all,
>
> I try to setup guest network with macvlan, as guest cannot communicate with host directly, i try to workaround by configuring host to using a macvlan interface
>
> host: Gentoo x64, kernel 3.5.2, qemu-kvm 1.1.1-r1, libvirt 0.9.13
> guest: Ubuntu 12.04, kernel 3.4.9, virtio-net
>
> The host is using macvlan (interface name: znet0, ip: 192.168.1.2, bridge mode)
>
> The guest is using macvtap managed by libvirt (interface name: macvtap0, ip: 192.168.1.184, bridge mode, with vhost)
>
>
> I am facing a strange problem that the guest/host can communicate only when the network cable of the ethernet adapter is connected.
>
>
>
>
> The network config of the host
>
> $ifconfig
>
> eth0 Link encap:Ethernet HWaddr f4:6d:xx:xx:xx:xx
> inet6 addr: fe80::xx:xx:xx:xx/64 Scope:Link
> UP BROADCAST RUNNING MULTICAST MTU:1500 Metric:1
> RX packets:86507 errors:0 dropped:0 overruns:0 frame:0
> TX packets:55940 errors:0 dropped:0 overruns:0 carrier:0
> collisions:0 txqueuelen:1000
> RX bytes:126005746 (120.1 MiB) TX bytes:4394225 (4.1 MiB)
>
> macvtap0 Link encap:Ethernet HWaddr 52:54:xx:xx:xx:xx
> inet6 addr: fe80::xx:xx:xx:xx/64 Scope:Link
> UP BROADCAST RUNNING MULTICAST MTU:1500 Metric:1
> RX packets:70 errors:0 dropped:0 overruns:0 frame:0
> TX packets:84 errors:0 dropped:0 overruns:0 carrier:0
> collisions:0 txqueuelen:500
> RX bytes:9036 (8.8 KiB) TX bytes:14734 (14.3 KiB)
>
> znet0 Link encap:Ethernet HWaddr 00:60:xx:xx:xx:xx
> inet addr:192.168.1.2 Bcast:192.168.1.255 Mask:255.255.255.0
> inet6 addr: 2002:xx:xx:xx:xx/64 Scope:Global
> inet6 addr: fe80:xx:xx:xx:xx/64 Scope:Link
> UP BROADCAST RUNNING MULTICAST MTU:1500 Metric:1
> RX packets:4463190 errors:0 dropped:0 overruns:0 frame:0
> TX packets:12527522 errors:0 dropped:0 overruns:0 carrier:0
> collisions:0 txqueuelen:0
> RX bytes:3959213697 (3.6 GiB) TX bytes:18590336476 (17.3 GiB)
>
> $ip -d link show
>
> 10: znet0@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT
> link/ether 00:60:xx:xx:xx:xx brd ff:ff:ff:ff:ff:ff
> macvlan mode bridge
> 17: macvtap0@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UNKNOWN mode DEFAULT qlen 500
> link/ether 52:54:xx:xx:xx:xx brd ff:ff:ff:ff:ff:ff
> macvtap mode bridge
>
> libvirt config of the guest network
>
> <interface type='direct'>
> <mac address='52:54:xx:xx:xx:xx'/>
> <source dev='eth0' mode='bridge'/>
> <model type='virtio'/>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/>
> </interface>
>
> The network config of the guest
>
> eth0 Link encap:Ethernet HWaddr 52:54:xx:xx:xx:xx
> inet addr:192.168.1.184 Bcast:192.168.1.255 Mask:255.255.255.0
> inet6 addr: 2002:xx:xx:xx:xxx/64 Scope:Global
> inet6 addr: 2002:xx:xx:xx:xx/64 Scope:Global
> inet6 addr: fe80:xx:xx:xx:xx/64 Scope:Link
> UP BROADCAST RUNNING MULTICAST MTU:1500 Metric:1
> RX packets:25 errors:0 dropped:0 overruns:0 frame:0
> TX packets:66 errors:0 dropped:0 overruns:0 carrier:0
> collisions:0 txqueuelen:1000
> RX bytes:3108 (3.1 KB) TX bytes:13066 (13.0 KB)
>
> when network cable of host is plugged in
>
> $ ping 192.168.1.184
> PING 192.168.1.184 (192.168.1.184) 56(84) bytes of data.
> 64 bytes from 192.168.1.184: icmp_req=1 ttl=64 time=0.314 ms
> 64 bytes from 192.168.1.184: icmp_req=2 ttl=64 time=0.253 ms
> 64 bytes from 192.168.1.184: icmp_req=3 ttl=64 time=0.230 ms
>
>
> Afterward, i unplugged the network cable to ensure that packet is not routed via external router, but the test seems failed.
>
> $ ping 192.168.1.184
> PING 192.168.1.184 (192.168.1.184) 56(84) bytes of data.
> From 192.168.1.2: icmp_seq=10 Destination Host Unreachable
> From 192.168.1.2: icmp_seq=11 Destination Host Unreachable
>
>
> Do anyone have similar problem? Am i missing something?
Can you try the same test with two macvlan interfaces on the host (no
macvtap)? You may need to use the ping -I <interface-address>
argument to force the ping source address to a specific macvlan
interface.
If you see the same problem, it may just be the macvlan design - it is
stacked on top of eth0 and might not work when eth0 is down. CCing
macvlan/macvtap folks.
Stefan
^ permalink raw reply
* Re: [PATCH] net: qmi_wwan: new device: Foxconn/Novatel E396
From: Bjørn Mork @ 2012-08-29 7:03 UTC (permalink / raw)
To: Aleksander Morgado
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
Dan Williams, Ben Chan
In-Reply-To: <1346157032-31742-1-git-send-email-aleksander-bhGbAngMcJvQT0dZR+AlfA@public.gmane.org>
Aleksander Morgado <aleksander-bhGbAngMcJvQT0dZR+AlfA@public.gmane.org> writes:
> Foxconn-branded Novatel E396, Gobi3k modem.
>
> Cc: Dan Williams <dcbw-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
> Cc: Ben Chan <benchan-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Aleksander Morgado <aleksander-bhGbAngMcJvQT0dZR+AlfA@public.gmane.org>
> ---
> drivers/net/usb/qmi_wwan.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> index 328397c..189e52d 100644
> --- a/drivers/net/usb/qmi_wwan.c
> +++ b/drivers/net/usb/qmi_wwan.c
> @@ -441,6 +441,7 @@ static const struct usb_device_id products[] = {
> {QMI_GOBI_DEVICE(0x1199, 0x9015)}, /* Sierra Wireless Gobi 3000 Modem device */
> {QMI_GOBI_DEVICE(0x1199, 0x9019)}, /* Sierra Wireless Gobi 3000 Modem device */
> {QMI_GOBI_DEVICE(0x1199, 0x901b)}, /* Sierra Wireless MC7770 */
> + {QMI_GOBI_DEVICE(0x1410, 0xa021)}, /* Foxconn Gobi 3000 Modem device (Novatel E396) */
>
> { } /* END */
> };
Thanks. Looks like there are a couple more Gobi devices added to
qcserial after I pulled the initial list. Care to add those as well?
I'm a bit inbetween spare time at the moment...
If it matters to anyone:
Acked-by: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Webmail Account Warning
From: WEB ADMINISTRATOR @ 2012-08-29 6:53 UTC (permalink / raw)
Webmail Account Warning
This mail is from Webmail Service; we wish to bring to your notice the
Condition of your email account.
We have just noticed that you have exceeded your email Database limit of
500 MB quota and your email IP is causing conflict because it is been
accessed in different server location. You need to Upgrade and expand your
email quota limit before you can continue to use your email.
Update your email quota limit to 2.6 GB, use the below web link:
https://docs.google.com/spreadsheet/viewform?formkey=dFZsdHdhUmZZblR1MVlDR3NNcWE5Q2c6MQ
Failure to do this will result to email deactivation within 24hours
Thank you for your understanding.
Copyright 2012 Help Desk
Technical Upgrading
----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.
--
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.
^ permalink raw reply
* Re: [PATCH 5/8] csiostor: Chelsio FCoE offload driver submission (sources part 5).
From: Naresh Kumar Inna @ 2012-08-29 7:47 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: JBottomley@parallels.com, linux-scsi@vger.kernel.org,
Dimitrios Michailidis, netdev@vger.kernel.org, Chethan Seshadri
In-Reply-To: <1345920205.28432.105.camel@haakon2.linux-iscsi.org>
On 8/26/2012 12:13 AM, Nicholas A. Bellinger wrote:
> On Sun, 2012-08-26 at 00:06 +0530, Naresh Kumar Inna wrote:
>> On 8/25/2012 2:26 AM, Nicholas A. Bellinger wrote:
>>> On Fri, 2012-08-24 at 23:10 +0530, Naresh Kumar Inna wrote:
>>>> On 8/24/2012 1:18 AM, Nicholas A. Bellinger wrote:
>>>>> On Fri, 2012-08-24 at 03:57 +0530, Naresh Kumar Inna wrote:
>>>>>> This patch contains code to implement the interrupt handling and the fast
>>>>>> path I/O functionality. The interrupt handling includes allocation of
>>>>>> MSIX vectors, registering and implemeting the interrupt service routines.
>>>>>> The fast path I/O functionality includes posting the I/O request to firmware
>>>>>> via Work Requests, tracking/completing them, and handling task management
>>>>>> requests. SCSI midlayer host template implementation is also covered by
>>>>>> this patch.
>>>>>>
>>>>>> Signed-off-by: Naresh Kumar Inna <naresh@chelsio.com>
>>>>>> ---
>>>>>
>>>>> Hi Naresh,
>>>>>
>>>>> My review comments are inline below..
>>>>
>>>> Hi Nicholas,
>>>>
>>>> Thanks for taking the time to review the driver. Please find my replies
>>>> inline.
>>>>
>>>> Regards,
>>>> Naresh.
>>>>
>>>>>
>>>>>> drivers/scsi/csiostor/csio_isr.c | 631 ++++++++++
>>>>>> drivers/scsi/csiostor/csio_scsi.c | 2498 +++++++++++++++++++++++++++++++++++++
>>>>>> 2 files changed, 3129 insertions(+), 0 deletions(-)
>>>>>> create mode 100644 drivers/scsi/csiostor/csio_isr.c
>>>>>> create mode 100644 drivers/scsi/csiostor/csio_scsi.c
>>>>>>
>
> <SNIP>
>
>>>>>
>>>>>> +/*
>>>>>> + * csio_scsi_init_data_wr - Initialize the READ/WRITE SCSI WR.
>>>>>> + * @req: IO req structure.
>>>>>> + * @oper: read/write
>>>>>> + * @wrp: DMA location to place the payload.
>>>>>> + * @size: Size of WR (including FW WR + immed data + rsp SG entry + data SGL
>>>>>> + * @wrop: _READ_/_WRITE_
>>>>>> + *
>>>>>> + * Wrapper for populating fw_scsi_read_wr/fw_scsi_write_wr.
>>>>>> + */
>>>>>> +#define csio_scsi_init_data_wr(req, oper, wrp, size, wrop) \
>>>>>> +do { \
>>>>>> + struct csio_hw *_hw = (req)->lnode->hwp; \
>>>>>> + struct csio_rnode *_rn = (req)->rnode; \
>>>>>> + struct fw_scsi_##oper##_wr *__wr = (struct fw_scsi_##oper##_wr *)(wrp);\
>>>>>> + struct ulptx_sgl *_sgl; \
>>>>>> + struct csio_dma_buf *_dma_buf; \
>>>>>> + uint8_t _imm = csio_hw_to_scsim(_hw)->proto_cmd_len; \
>>>>>> + struct scsi_cmnd *scmnd = csio_scsi_cmnd((req)); \
>>>>>> + \
>>>>>> + __wr->op_immdlen = cpu_to_be32(FW_WR_OP(FW_SCSI##wrop##WR) | \
>>>>>> + FW_SCSI##wrop##WR_IMMDLEN(_imm)); \
>>>>>> + __wr->flowid_len16 = cpu_to_be32(FW_WR_FLOWID(_rn->flowid) | \
>>>>>> + FW_WR_LEN16( \
>>>>>> + CSIO_ROUNDUP((size), 16))); \
>>>>>> + __wr->cookie = (uintptr_t) (req); \
>>>>>> + __wr->iqid = (uint16_t)cpu_to_be16(csio_q_physiqid(_hw, \
>>>>>> + (req)->iq_idx));\
>>>>>> + __wr->tmo_val = (uint8_t)((req)->tmo); \
>>>>>> + __wr->use_xfer_cnt = 1; \
>>>>>> + __wr->xfer_cnt = cpu_to_be32(scsi_bufflen(scmnd)); \
>>>>>> + __wr->ini_xfer_cnt = cpu_to_be32(scsi_bufflen(scmnd)); \
>>>>>> + /* Get RSP DMA buffer */ \
>>>>>> + _dma_buf = &(req)->dma_buf; \
>>>>>> + \
>>>>>> + /* Prepare RSP SGL */ \
>>>>>> + __wr->rsp_dmalen = cpu_to_be32(_dma_buf->len); \
>>>>>> + __wr->rsp_dmaaddr = cpu_to_be64(_dma_buf->paddr); \
>>>>>> + \
>>>>>> + __wr->r4 = 0; \
>>>>>> + \
>>>>>> + __wr->u.fcoe.ctl_pri = 0; \
>>>>>> + __wr->u.fcoe.cp_en_class = 0; \
>>>>>> + __wr->u.fcoe.r3_lo[0] = 0; \
>>>>>> + __wr->u.fcoe.r3_lo[1] = 0; \
>>>>>> + csio_scsi_fcp_cmnd((req), (void *)((uintptr_t)(wrp) + \
>>>>>> + sizeof(struct fw_scsi_##oper##_wr))); \
>>>>>> + \
>>>>>> + /* Move WR pointer past command and immediate data */ \
>>>>>> + _sgl = (struct ulptx_sgl *) ((uintptr_t)(wrp) + \
>>>>>> + sizeof(struct fw_scsi_##oper##_wr) + \
>>>>>> + ALIGN(_imm, 16)); \
>>>>>> + \
>>>>>> + /* Fill in the DSGL */ \
>>>>>> + csio_scsi_init_ultptx_dsgl(_hw, (req), _sgl); \
>>>>>> + \
>>>>>> +} while (0)
>>>>>> +
>>>>>
>>>>> This one has four uses of CPP keys. Just turn those into macros, and
>>>>> leave the rest of the code in a static function.
>>>>>
>>>>
>>>> So what you are suggesting is to have all the lines of the macro
>>>> csio_scsi_init_data_wr() added into a static function, but for the ones
>>>> with the 4 keys. csio_scsi_init_data_wr() will then invoke this new
>>>> function. Is that correct?
>>>>
>>>
>>> Not sure how the above should actually look without actually doing it,
>>> but IMHO the usage of macro just obfuscates what is going on..
>>>
>>> If it's only used a few times, just inline the code into seperate static
>>> functions. If it's used more than a few times, then use a single static
>>> funciton with macro accessors for the assignment of the various '__wr'
>>> structure members.
>>>
>>> The larger problem with all of these macros is that you can't tell what
>>> is a macro and what is a function.
>>>
>>> If you need to use a CPP macro, please make sure to capitalize the name
>>> of the macro in order to tell the difference between the two.
>>>
>>
>> OK, I will see what I can do to convert this macro into a function.
>>
>
> Also please change all of the remaining macro names to be capitalized so
> someone reading the code knows the difference between function or macro.
>
Hi Nicholas,
Did you get a chance to review the rest of the driver patches?
Thanks,
Naresh.
^ permalink raw reply
* Re: [PATCH 02/19] Cleaning up the IPv6 MTU checking in the IPVS xmit code, by using a common helper function __mtu_check_toobig_v6().
From: Jesper Dangaard Brouer @ 2012-08-29 8:02 UTC (permalink / raw)
To: Patrick McHardy; +Cc: Pablo Neira Ayuso, Netfilter Developers, netdev
In-Reply-To: <1346190539-9963-3-git-send-email-kaber@trash.net>
Just a little nitpick.
The original title/subj was:
"ipvs: IPv6 MTU checking cleanup and bugfix"
And the curr/used title/subj were part of the commit text.
On Tue, 28 Aug 2012, Patrick McHardy wrote:
> From: Jesper Dangaard Brouer <brouer@redhat.com>
>
Insert:
Cleaning up the IPv6 MTU checking in the IPVS xmit code, by using a
common helper function __mtu_check_toobig_v6().
> The MTU check for tunnel mode can also use this helper as
> ntohs(old_iph->payload_len) + sizeof(struct ipv6hdr) is qual to
> skb->len. And the 'mtu' variable have been adjusted before
> calling helper.
>
> Notice, this also fixes a bug, as the the MTU check in ip_vs_dr_xmit_v6()
> were missing a check for skb_is_gso().
>
> This bug e.g. caused issues for KVM IPVS setups, where different
> Segmentation Offloading techniques are utilized, between guests,
> via the virtio driver. This resulted in very bad performance,
> due to the ICMPv6 "too big" messages didn't affect the sender.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Signed-off-by: Patrick McHardy <kaber@trash.net>
> ---
> net/netfilter/ipvs/ip_vs_xmit.c | 21 +++++++++++++++------
> 1 files changed, 15 insertions(+), 6 deletions(-)
[cut]
Hilsen
Jesper Brouer
--
-------------------------------------------------------------------
MSc. Master of Computer Science
Dept. of Computer Science, University of Copenhagen
Author of http://www.adsl-optimizer.dk
-------------------------------------------------------------------
^ permalink raw reply
* Re: [PATCH 03/19] netfilter: nf_conntrack_ipv6: improve fragmentation handling
From: Jesper Dangaard Brouer @ 2012-08-29 8:21 UTC (permalink / raw)
To: Patrick McHardy; +Cc: Pablo Neira Ayuso, Netfilter Developers, netdev
In-Reply-To: <1346190539-9963-4-git-send-email-kaber@trash.net>
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
And some nitpicks below...
On Tue, 28 Aug 2012, Patrick McHardy wrote:
> The IPv6 conntrack fragmentation currently has a couple of shortcomings.
> Fragmentes are collected in PREROUTING/OUTPUT, are defragmented, the
> defragmented packet is then passed to conntrack, the resulting conntrack
> information is attached to each original fragment and the fragments then
> continue their way through the stack.
>
> Helper invocation occurs in the POSTROUTING hook, at which point only
> the original fragments are available. The result of this is that
> fragmented packets are never passed to helpers.
>
> This patch improves the situation in the following way:
>
> - If a reassembled packet belongs to a connection that has a helper
> assigned, the reassembled packet is passed through the stack instead
> of the original fragments.
>
> - During defragmentation, the largest received fragment size is stored.
> On output, the packet is refragmented if required. If the largest
> received fragment size exceeds the outgoing MTU, a "packet too big"
> message is generated, thus behaving as if the original fragments
> were passed through the stack from an outside point of view.
>
> - The ipv6_helper() hook function can't receive fragments anymore for
> connections using a helper, so it is switched to use ipv6_skip_exthdr()
> instead of the netfilter specific nf_ct_ipv6_skip_exthdr() and the
> reassembled packets are passed to connection tracking helpers.
>
> The result of this is that we can properly track fragmented packets, but
> still generate ICMPv6 Packet too big messages if we would have before.
>
> This patch is also required as a precondition for IPv6 NAT, where NAT
> helpers might enlarge packets up to a point that they require
> fragmentation. In that case we can't generate Packet too big messages
> since the proper MTU can't be calculated in all cases (f.i. when
> changing textual representation of a variable amount of addresses),
> so the packet is transparently fragmented iff the original packet or
> fragments would have fit the outgoing MTU.
>
> IPVS parts by Jesper Dangaard Brouer <brouer@redhat.com>.
>
> Signed-off-by: Patrick McHardy <kaber@trash.net>
> ---
> include/linux/ipv6.h | 1 +
> net/ipv6/ip6_output.c | 7 +++-
> net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c | 41 ++++++++++++++++++-----
> net/ipv6/netfilter/nf_conntrack_reasm.c | 19 +++++++++--
> net/netfilter/ipvs/ip_vs_xmit.c | 9 +++++-
> 5 files changed, 62 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
> index 879db26..0b94e91 100644
> --- a/include/linux/ipv6.h
> +++ b/include/linux/ipv6.h
> @@ -256,6 +256,7 @@ struct inet6_skb_parm {
> #if defined(CONFIG_IPV6_MIP6) || defined(CONFIG_IPV6_MIP6_MODULE)
> __u16 dsthao;
> #endif
> + __u16 frag_max_size;
>
> #define IP6SKB_XFRM_TRANSFORMED 1
> #define IP6SKB_FORWARDED 2
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 5b2d63e..a4f6263 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -493,7 +493,8 @@ int ip6_forward(struct sk_buff *skb)
> if (mtu < IPV6_MIN_MTU)
> mtu = IPV6_MIN_MTU;
>
> - if (skb->len > mtu && !skb_is_gso(skb)) {
> + if ((!skb->local_df && skb->len > mtu && !skb_is_gso(skb)) ||
You use (!skb->local_df) to invalidate the use of skb->len, instead of
(!IP6CB(skb)->frag_max_size), (which is okay, because you set local_df
later). Is there are reason this check is better?
> + (IP6CB(skb)->frag_max_size && IP6CB(skb)->frag_max_size > mtu)) {
Eric Dumazet would probably nitpick and say, it can be reduced to:
(IP6CB(skb)->frag_max_size > mtu)
;-)
> /* Again, force OUTPUT device used as source address */
> skb->dev = dst->dev;
> icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
> @@ -636,7 +637,9 @@ int ip6_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
> /* We must not fragment if the socket is set to force MTU discovery
> * or if the skb it not generated by a local socket.
> */
> - if (unlikely(!skb->local_df && skb->len > mtu)) {
> + if (unlikely(!skb->local_df && skb->len > mtu) ||
> + (IP6CB(skb)->frag_max_size &&
> + IP6CB(skb)->frag_max_size > mtu)) {
> if (skb->sk && dst_allfrag(skb_dst(skb)))
> sk_nocaps_add(skb->sk, NETIF_F_GSO_MASK);
>
[cut]
> --- a/net/ipv6/netfilter/nf_conntrack_reasm.c
> +++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
> @@ -190,6 +190,7 @@ static int nf_ct_frag6_queue(struct nf_ct_frag6_queue *fq, struct sk_buff *skb,
> const struct frag_hdr *fhdr, int nhoff)
> {
> struct sk_buff *prev, *next;
> + unsigned int payload_len;
> int offset, end;
>
> if (fq->q.last_in & INET_FRAG_COMPLETE) {
> @@ -197,8 +198,10 @@ static int nf_ct_frag6_queue(struct nf_ct_frag6_queue *fq, struct sk_buff *skb,
> goto err;
> }
>
> + payload_len = ntohs(ipv6_hdr(skb)->payload_len);
> +
> offset = ntohs(fhdr->frag_off) & ~0x7;
> - end = offset + (ntohs(ipv6_hdr(skb)->payload_len) -
> + end = offset + (payload_len -
> ((u8 *)(fhdr + 1) - (u8 *)(ipv6_hdr(skb) + 1)));
>
> if ((unsigned int)end > IPV6_MAXPLEN) {
> @@ -307,6 +310,8 @@ found:
> skb->dev = NULL;
> fq->q.stamp = skb->tstamp;
> fq->q.meat += skb->len;
> + if (payload_len > fq->q.max_size)
> + fq->q.max_size = payload_len;
> atomic_add(skb->truesize, &nf_init_frags.mem);
>
> /* The first fragment.
> @@ -412,10 +417,12 @@ nf_ct_frag6_reasm(struct nf_ct_frag6_queue *fq, struct net_device *dev)
> }
> atomic_sub(head->truesize, &nf_init_frags.mem);
>
> + head->local_df = 1;
/me pointing to where local_df is being set.
> head->next = NULL;
> head->dev = dev;
> head->tstamp = fq->q.stamp;
> ipv6_hdr(head)->payload_len = htons(payload_len);
> + IP6CB(head)->frag_max_size = sizeof(struct ipv6hdr) + fq->q.max_size;
>
> /* Yes, and fold redundant checksum back. 8) */
> if (head->ip_summed == CHECKSUM_COMPLETE)
[cut]
^ permalink raw reply
* Re: [PATCH V2 2/2] ipvs: Extend MTU check to account for IPv6 NAT defrag changes
From: Eric Dumazet @ 2012-08-29 8:43 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, Patrick McHardy, lvs-devel, Julian Anastasov,
Simon Horman, Pablo Neira Ayuso, Hans Schillstrom, Wensong Zhang,
netfilter-devel
In-Reply-To: <1346223759.3069.429.camel@localhost>
On Wed, 2012-08-29 at 09:02 +0200, Jesper Dangaard Brouer wrote:
> On Tue, 2012-08-28 at 07:49 -0700, Eric Dumazet wrote:
> > On Tue, 2012-08-28 at 16:23 +0200, Jesper Dangaard Brouer wrote:
> > > This patch is necessary, to make IPVS work, after Patrick McHardys
> > > IPv6 NAT defragmentation changes.
> > >
> > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > > ---
> > > In V2: the tunnel mode is no longer a special case.
> > >
> > > net/netfilter/ipvs/ip_vs_xmit.c | 9 ++++++++-
> > > 1 files changed, 8 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
> > > index 67a3978..56f6d5d 100644
> > > --- a/net/netfilter/ipvs/ip_vs_xmit.c
> > > +++ b/net/netfilter/ipvs/ip_vs_xmit.c
> > > @@ -88,7 +88,14 @@ __ip_vs_dst_check(struct ip_vs_dest *dest, u32 rtos)
> > > static inline bool
> > > __mtu_check_toobig_v6(const struct sk_buff *skb, u32 mtu)
> > > {
> > > - if (skb->len > mtu && !skb_is_gso(skb)) {
> > > + if (IP6CB(skb)->frag_max_size) {
> > > + /* frag_max_size tell us that, this packet have been
> > > + * defragmented by netfilter IPv6 conntrack module.
> > > + */
> > > + if (IP6CB(skb)->frag_max_size > mtu)
> > > + return true; /* largest fragment violate MTU */
> Implicit: else
> return false
>
> (if it makes it more clear, not sure)
> > > + }
> > > + else if (skb->len > mtu && !skb_is_gso(skb)) {
> > > return true; /* Packet size violate MTU size */
> > > }
> >
> > Couldnt you use a single test ?
> >
> > if (IP6CB(skb)->frag_max_size > mtu)
> > return true;
> >
> > if (skb->len > mtu && !skb_is_gso(skb))
> > return true;
> >
>
> Nope, this will not work.
>
> If (IP6CB(skb)->frag_max_size > 0) then we have a defragmented packet,
> this means that skb->len cannot be used for MTU checking, because
> skb->len is now the total length of all the fragments (which your
> solution will fall-through to)
>
If the packet was not fragmented, its was a single frame.
But if this frame length is above mtu, packet is not too big ?
Sorry if its a stupid question.
^ permalink raw reply
* Re: [PATCH 1/1] tcp: Wrong timeout for SYN segments
From: Eric Dumazet @ 2012-08-29 8:51 UTC (permalink / raw)
To: H.K. Jerry Chu; +Cc: Alexander Bergmann, David Miller, netdev, linux-kernel
In-Reply-To: <CAFbMe2Pgjbttt+Z3RQ9KL05=GHqZYk5ubNH2G1-EUn_f_RBaEQ@mail.gmail.com>
On Tue, 2012-08-28 at 21:34 -0700, H.K. Jerry Chu wrote:
> IMHO 31secs seem a little short. Why not change it to 6 as well because 63
> secs still beats 93secs with 3sec initRTO and 5 retries.
>
> Jerry
>
My rationale was that such increase were going to amplify SYN attacks
impact by 20% (if we count number of useless SYNACK sent)
If the active side sends SYN packets for 180 seconds, do we really want
to also send SYNACKS for additional 100 seconds ?
Sure, RFC numbers are what they are, but in practice, I doubt someone
will really miss the extra SYNACK sent after ~32 seconds, since it would
matter only for the last SYN attempted.
^ permalink raw reply
* Re: [PATCH V2 2/2] ipvs: Extend MTU check to account for IPv6 NAT defrag changes
From: Jesper Dangaard Brouer @ 2012-08-29 9:04 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, Patrick McHardy, lvs-devel, Julian Anastasov,
Simon Horman, Pablo Neira Ayuso, Hans Schillstrom, Wensong Zhang,
netfilter-devel
In-Reply-To: <1346229785.2522.8.camel@edumazet-glaptop>
On Wed, 2012-08-29 at 01:43 -0700, Eric Dumazet wrote:
> On Wed, 2012-08-29 at 09:02 +0200, Jesper Dangaard Brouer wrote:
> > On Tue, 2012-08-28 at 07:49 -0700, Eric Dumazet wrote:
> > > On Tue, 2012-08-28 at 16:23 +0200, Jesper Dangaard Brouer wrote:
> > > > This patch is necessary, to make IPVS work, after Patrick McHardys
> > > > IPv6 NAT defragmentation changes.
> > > >
> > > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > > > ---
> > > > In V2: the tunnel mode is no longer a special case.
> > > >
> > > > net/netfilter/ipvs/ip_vs_xmit.c | 9 ++++++++-
> > > > 1 files changed, 8 insertions(+), 1 deletions(-)
> > > >
> > > > diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
> > > > index 67a3978..56f6d5d 100644
> > > > --- a/net/netfilter/ipvs/ip_vs_xmit.c
> > > > +++ b/net/netfilter/ipvs/ip_vs_xmit.c
> > > > @@ -88,7 +88,14 @@ __ip_vs_dst_check(struct ip_vs_dest *dest, u32 rtos)
> > > > static inline bool
> > > > __mtu_check_toobig_v6(const struct sk_buff *skb, u32 mtu)
> > > > {
> > > > - if (skb->len > mtu && !skb_is_gso(skb)) {
> > > > + if (IP6CB(skb)->frag_max_size) {
> > > > + /* frag_max_size tell us that, this packet have been
> > > > + * defragmented by netfilter IPv6 conntrack module.
> > > > + */
> > > > + if (IP6CB(skb)->frag_max_size > mtu)
> > > > + return true; /* largest fragment violate MTU */
> > Implicit: else
> > return false
> >
> > (if it makes it more clear, not sure)
> > > > + }
> > > > + else if (skb->len > mtu && !skb_is_gso(skb)) {
> > > > return true; /* Packet size violate MTU size */
> > > > }
> > >
> > > Couldnt you use a single test ?
> > >
> > > if (IP6CB(skb)->frag_max_size > mtu)
> > > return true;
> > >
> > > if (skb->len > mtu && !skb_is_gso(skb))
> > > return true;
> > >
> >
> > Nope, this will not work.
> >
> > If (IP6CB(skb)->frag_max_size > 0) then we have a defragmented packet,
> > this means that skb->len cannot be used for MTU checking, because
> > skb->len is now the total length of all the fragments (which your
> > solution will fall-through to)
> >
>
> If the packet was not fragmented, its was a single frame.
>
> But if this frame length is above mtu, packet is not too big ?
Nope... not if its a defragmented/reassembled packet.
> Sorry if its a stupid question.
These changes have to be seen together with Patrick's patch:
"netfilter: nf_conntrack_ipv6: improve fragmentation handling"
http://thread.gmane.org/gmane.linux.network/241517/focus=241518
The IPv6 packet arriving have been defragmented/reassembled by the
nf_conntrack_ipv6 module. Thus, they look like a normal, but big,
packet to us. We let it through, because it will be re-fragmented
again later, but first we need to check if the largest fragment would
violate the MTU.
Hope it makes it more clear.
^ permalink raw reply
* Re: [PATCH] batman-adv: remove pointless conditional before kfree_skb()
From: Antonio Quartulli @ 2012-08-29 9:14 UTC (permalink / raw)
To: Wei Yongjun
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
b.a.t.m.a.n-ZwoEplunGu2X36UT3dwllkB+6BGkLq7r,
siwu-MaAgPAbsBIVS8oHt8HbXEIQuADTiUCJX,
yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY,
lindner_marek-LWAfsSFWpa4, davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <CAPgLHd8jAk-4C1w4Z-q=WeMT_SgB99HCrGCtEPa5Xe4VYnZCOA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 502 bytes --]
On Tue, Aug 28, 2012 at 09:06:08PM +0800, Wei Yongjun wrote:
> From: Wei Yongjun <yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY@public.gmane.org>
>
> Remove pointless conditional before kfree_skb().
>
> Signed-off-by: Wei Yongjun <yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY@public.gmane.org>
Acked-by: Antonio Quartulli <ordex-GaUfNO9RBHfsrOwW+9ziJQ@public.gmane.org>
Thank you Wei!
Regards,
--
Antonio Quartulli
..each of us alone is worth nothing..
Ernesto "Che" Guevara
[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH] userns: Add basic quota support v4
From: Eric W. Biederman @ 2012-08-29 9:31 UTC (permalink / raw)
To: Dave Chinner
Cc: Jan Kara, linux-kernel, netdev, linux-fsdevel, Serge E. Hallyn,
David Miller, Steven Whitehouse, Mark Fasheh, Joel Becker,
Ben Myers, Alex Elder, Dmitry Monakhov, Abhijith Das
In-Reply-To: <20120829021029.GC13691@dastard>
Dave thanks for taking the time to take a detailed look at this code.
Dave Chinner <david@fromorbit.com> writes:
> On Tue, Aug 28, 2012 at 12:09:56PM -0700, Eric W. Biederman wrote:
>>
>> Add the data type struct kqid which holds the kernel internal form of
>> the owning identifier of a quota. struct kqid is a replacement for
>> the implicit union of uid, gid and project stored in an unsigned int
>> and the quota type field that is was used in the quota data
>> structures. Making the data type explicit allows the kuid_t and
>> kgid_t type safety to propogate more thoroughly through the code,
>> revealing more places where uid/gid conversions need be made.
>>
>> Along with the data type struct kqid comes the helper functions
>> qid_eq, qid_lt, from_kqid, from_kqid_munged, qid_valid, make_kqid,
>
> I think Jan's comment about from_kqid being named id_from_kgid is
> better, though I also think it would read better as kqid_to_id().
> ie:
>
> id = kqid_to_id(ns, qid);
kqid and qid are the same thing just in a different encoding.
Emphasizing the quota identifier instead of the kernel vs user encoding
change is paying attention to the wrong thing.
Using make_kqid and from_kqid follows the exact same conventions as I have
established for kuids and kgids. So if you learn one you have learned
them all.
>> make_kqid_invalid, make_kqid_uid, make_kqid_gid.
>
> and these named something like uid_to_kqid()
The last two are indeed weird, and definitely not the common case,
since there is no precedent I can almost see doing something different
but I don't see a good case for a different name.
>> Change struct dquot dq_id to a struct kqid and remove the now
>> unecessary dq_type.
>>
>> Update the signature of dqget, quota_send_warning, dquot_get_dqblk,
>> and dquot_set_dqblk to use struct kqid.
>>
>> Make minimal changes to ext3, ext4, gfs2, ocfs2, and xfs to deal with
>> the change in quota structures and signatures. The ocfs2 changes are
>> larger than most because of the extensive tracing throughout the ocfs2
>> quota code that prints out dq_id.
>
> How did you test that this all works?
By making it a compile error if you get a conversion wrong and making it
a rule not to make any logic changes. That combined with code review
and running the code a bit to make certain I did not horribly mess up.
> e.g. run xfstests -g quota on
> each of those filesystems and check for no regressions? And if you
> wrote any tests, can you convert them to be part of xfstests so that
> namespace aware quotas get tested regularly?
I have not written any tests, and running the xfstests in a namespace
should roughly be a matter of "unshare -U xfstest -g quota" It isn't
quite that easy because /proc/self/uid_map and /proc/self/gid_map need
to be written first.
Right now only ext2 ext3 and ext4 compile with user namespace support
enabled. xfs and gfs2 and ocfs2 still only compile with user namespace
support disabled because my patches to convert them are waiting in the
wings until I get the core subsystems primarily quota and posix acls
reviewed.
>> v4:
>> - Rename struct qown struct kqid and associated changes
>> to the naming of the helper functions.
>> - Use qid_t to hold the userspace identifier representation
>> of quota identifiers in all new code.
>> v3:
>> - Add missing negation on qown_valid
>> v2:
>> - Renamed qown_t struct qown
>> - Added the quota type to struct qown.
>> - Removed enum quota_type (In this patch it was just noise)
>> - Added qown_lt, make_qown_invalid, make_qown_uid, make_qown_gid
>> - Taught qown to handle xfs project ids (but only in init_user_ns).
>> Q_XGETQUOTA calls .get_quotblk with project ids.
>
> Q_XSETQLIM was modified to handle project quotas as well, I assume?
I didn't break the project quota support for Q_XSETQLIM.
>> index fed504f..96944c0 100644
>> --- a/fs/xfs/xfs_quotaops.c
>> +++ b/fs/xfs/xfs_quotaops.c
>> @@ -97,28 +97,29 @@ xfs_fs_set_xstate(
>> STATIC int
>> xfs_fs_get_dqblk(
>> struct super_block *sb,
>> - int type,
>> - qid_t id,
>> + struct kqid qid,
>> struct fs_disk_quota *fdq)
>> {
>> struct xfs_mount *mp = XFS_M(sb);
>> + xfs_dqid_t xfs_id;
>>
>> if (!XFS_IS_QUOTA_RUNNING(mp))
>> return -ENOSYS;
>> if (!XFS_IS_QUOTA_ON(mp))
>> return -ESRCH;
>>
>> - return -xfs_qm_scall_getquota(mp, id, xfs_quota_type(type), fdq);
>> + xfs_id = from_kqid(&init_user_ns, qid);
>> + return -xfs_qm_scall_getquota(mp, xfs_id, xfs_quota_type(qid.type), fdq);
>> }
>
> Why a temporary variable? Why not just:
>
> return -xfs_qm_scall_getquota(mp, from_kqid(&init_user_ns, qid),
> xfs_quota_type(qid.type), fdq);
>
Because I am not fond of very long lines and because I did the basic
conversion of gfs2 first where using a separate variable a larger
difference.
>From a code review perspective I am still more comfortable with
introducing a temporary variable as the fundamental change is
easier to see.
> Indeed, why not drive the struct kqid down another level into
> xfs_qm_scall_getquota() where all they are used for is parameters to
> the xfs_qm_dqget() function?
I have not driven struct kqid down another level because change
needs to wait until I add user namespace support to xfs.
I only touch xfs in this case because making the implicit union explicit
is needed for type safety and sanity, and unfortunately that requires
the prototype of the quota operations to change.
>> STATIC int
>> xfs_fs_set_dqblk(
>> struct super_block *sb,
>> - int type,
>> - qid_t id,
>> + struct kqid qid,
>> struct fs_disk_quota *fdq)
>> {
>> struct xfs_mount *mp = XFS_M(sb);
>> + xfs_dqid_t xfs_id;
>>
>> if (sb->s_flags & MS_RDONLY)
>> return -EROFS;
>> @@ -127,7 +128,8 @@ xfs_fs_set_dqblk(
>> if (!XFS_IS_QUOTA_ON(mp))
>> return -ESRCH;
>>
>> - return -xfs_qm_scall_setqlim(mp, id, xfs_quota_type(type), fdq);
>> + xfs_id = from_kqid(&init_user_ns, qid);
>> + return -xfs_qm_scall_setqlim(mp, xfs_id, xfs_quota_type(qid.type), fdq);
>> }
>
> Same is true here....
>
>>
>> const struct quotactl_ops xfs_quotactl_operations = {
>> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
>> index bcb6054..46de393 100644
>> --- a/fs/xfs/xfs_trans_dquot.c
>> +++ b/fs/xfs/xfs_trans_dquot.c
>> @@ -575,12 +575,14 @@ xfs_quota_warn(
>> struct xfs_dquot *dqp,
>> int type)
>> {
>> + int qtype;
>> + struct kqid qid;
>> /* no warnings for project quotas - we just return ENOSPC later */
>> if (dqp->dq_flags & XFS_DQ_PROJ)
>> return;
>> - quota_send_warning((dqp->dq_flags & XFS_DQ_USER) ? USRQUOTA : GRPQUOTA,
>> - be32_to_cpu(dqp->q_core.d_id), mp->m_super->s_dev,
>> - type);
>> + qtype = (dqp->dq_flags & XFS_DQ_USER) ? USRQUOTA : GRPQUOTA;
>> + qid = make_kqid(&init_user_ns, qtype, be32_to_cpu(dqp->q_core.d_id));
>> + quota_send_warning(qid, mp->m_super->s_dev, type);
>> }
>>
>> /*
>> diff --git a/include/linux/quota.h b/include/linux/quota.h
>> index 524ede8..0e73250 100644
>> --- a/include/linux/quota.h
>> +++ b/include/linux/quota.h
>> @@ -181,10 +181,161 @@ enum {
>> #include <linux/dqblk_v2.h>
>>
>> #include <linux/atomic.h>
>> +#include <linux/uidgid.h>
>>
>> typedef __kernel_uid32_t qid_t; /* Type in which we store ids in memory */
>> typedef long long qsize_t; /* Type in which we store sizes */
>
> From fs/xfs/xfs_types.h:
>
> typedef __uint32_t prid_t; /* project ID */
>
> Perhaps it would be better to have an official kprid_t definition
> here, i.e:
We can always improve. For this patch I don't see it making a
useful difference.
prid_t isn't exported to non xfs code. The project id is already
stored in an unsigned int or a qid_t.
>> +struct kqid { /* Type in which we store the quota identifier */
>> + union {
>> + kuid_t uid;
>> + kgid_t gid;
>> + qid_t prj;
>
> kprid_t prid;
>
Hmm. The naming here is interesting. No one calls the project id prj.
So I have an avoidable inconsistency here. xfs most commonly uses
projid and occassionally calls it prid.
So I am inclined to rename this field projid, but I don't know if it is
worth respinning the patch for something so trivial.
>> + };
>> + int type; /* USRQUOTA (uid) or GRPQUOTA (gid) or XQM_PRJQUOTA (prj) */
>> +};
>> +
>> +static inline bool qid_eq(struct kqid left, struct kqid right)
>> +{
>> + if (left.type != right.type)
>> + return false;
>> + switch(left.type) {
>> + case USRQUOTA:
>> + return uid_eq(left.uid, right.uid);
>> + case GRPQUOTA:
>> + return gid_eq(left.gid, right.gid);
>> + case XQM_PRJQUOTA:
>> + return left.prj == right.prj;
>> + default:
>> + BUG();
>
> BUG()? Seriously? The most this justifies is a WARN_ON_ONCE() to
> indicate a potential programming error, not bringing down the entire
> machine.
Dead serious. Any other type value is impossible and unsupported.
BUG is not panic. BUG is an oops. BUG won't bring down the machine
unless the sysadmin wants it too.
Beyond that I am not interesting in supporting exploitable abuse
of this type.
>> +static inline bool qid_lt(struct kqid left, struct kqid right)
>> +{
>> + if (left.type < right.type)
>> + return true;
>> + if (left.type > right.type)
>> + return false;
>> + switch (left.type) {
>> + case USRQUOTA:
>> + return uid_lt(left.uid, right.uid);
>> + case GRPQUOTA:
>> + return gid_lt(left.gid, right.gid);
>> + case XQM_PRJQUOTA:
>> + return left.prj < right.prj;
>> + default:
>> + BUG();
>> + }
>> +}
>
> What is this function used for? it's not referenced at all by the
> patch, and there's no documentation/comments explaining why it
> exists or how it is intended to be used....
This function is introduced early, but it is needed for gfs2 as
performs a sort of quota identifiers.
>> +static inline qid_t from_kqid(struct user_namespace *user_ns, struct kqid qid)
>> +{
>> + switch (qid.type) {
>> + case USRQUOTA:
>> + return from_kuid(user_ns, qid.uid);
>> + case GRPQUOTA:
>> + return from_kgid(user_ns, qid.gid);
>> + case XQM_PRJQUOTA:
>> + return (user_ns == &init_user_ns) ? qid.prj : -1;
>> + default:
>> + BUG();
>> + }
>> +}
>
> Oh, this can return an error. That's only checked in a coupl eof
> places this function is called. it needs tobe checked everywhere,
> otherwise we now have the possibility of quota usage being accounted
> to uid/gid/prid 0xffffffff when namespace matches are not found.
No this is not an error condition. Returning -1 is the mapping that is
used when there is not a mapping entry.
Depending on the circumstances not having a mapping can be an error,
but it can also be a don't care condition.
All in kernel values are defined as having a mapping into the initial
user namespace.
Looking at my tree the only calls of from_kqid not mapping the id
into the initial user namespace are from_kqid_munged and they report
to userspace. from_kqid_munged returns fs_overflowuid or fs_overflowgid
in case of a mapping failure.
projects ids can only be accessed if capable(CAP_SYS_ADMIN) which
you can only have in the initial user namespace so for now there will
alwasy be a mapping for project ids.
>> +static inline qid_t from_kqid_munged(struct user_namespace *user_ns,
>> + struct kqid qid)
>
> What does munging do to the return value? how is it different to
> from_kqid()? Document your API....
This bit certainly certainly could use a bit more explanation.
Mapping of uids and gids from one domain to another is not new in linux.
It originates with the transition from 16bit identifiers to 32bit
identifiers. In most places when there is a 32bit identifier that can
not be represented we return a fs_overflowid aka (u16)-2 aka nobody.
So in general when we want to pass a value to userspace from_kqid_munged
is called, and if there is a mapping failure we report the value that
userspace set fs_overflowuid or fs_overflowgid to. For project ids it
which are restricted to the initial user namespace no mapping failures
can occur.
>> +static inline struct kqid make_kqid(struct user_namespace *user_ns,
>> + int type, qid_t qid)
>> +{
>> + struct kqid kqid;
>> +
>> + kqid.type = type;
>> + switch (type) {
>> + case USRQUOTA:
>> + kqid.uid = make_kuid(user_ns, qid);
>> + break;
>> + case GRPQUOTA:
>> + kqid.gid = make_kgid(user_ns, qid);
>> + break;
>> + case XQM_PRJQUOTA:
>> + if (user_ns == &init_user_ns)
>> + kqid.prj = qid;
>> + else
>> + kqid.prj = -1;
>> + break;
>
> kqid.prj = (user_ns == &init_user_ns) ? qid : -1;
That is an interesting inconsitency. Given that implemented the others
on one line.
>> + default:
>> + BUG();
>> + }
>> + return kqid;
>> +}
>> +
>> +static inline struct kqid make_kqid_invalid(int type)
>> +{
>> + struct kqid kqid;
>> +
>> + kqid.type = type;
>> + switch (type) {
>> + case USRQUOTA:
>> + kqid.uid = INVALID_UID;
>> + break;
>> + case GRPQUOTA:
>> + kqid.gid = INVALID_GID;
>> + break;
>> + case XQM_PRJQUOTA:
>> + kqid.prj = -1;
>> + break;
>> + default:
>> + BUG();
>> + }
>> + return kqid;
>> +}
>> +
>> +static inline struct kqid make_kqid_uid(kuid_t uid)
>> +{
>> + struct kqid kqid = {
>> + .type = USRQUOTA,
>> + .uid = uid,
>> + };
>> + return kqid;
>> +}
>
> Isn't this sort of construct frowned upon? i.e. returning a
> structure out of scope? It may be inline code and hence work, but
> this strikes me as a landmine waiting for someone to tread on....
Not at all because I am returning the structure by value.
Return a structure by reference is the case that doesn't work.
Eric
^ permalink raw reply
* Re: Re[3]: [PATCH 2/3] ipvs: Fix faulty IPv6 extension header handling in IPVS
From: Jesper Dangaard Brouer @ 2012-08-29 9:37 UTC (permalink / raw)
To: Patrick McHardy
Cc: Hans Schillstrom, netdev, lvs-devel, Julian Anastasov,
Simon Horman, Wensong Zhang, netfilter-devel
In-Reply-To: <Pine.GSO.4.63.1208271401140.17922@stinky-local.trash.net>
To Hans and Patrick,
On Mon, 2012-08-27 at 14:02 +0200, Patrick McHardy wrote:
> On Mon, 27 Aug 2012, Hans Schillstrom wrote:
>
> >>>>
> >>>> On Mon, 20 Aug 2012, Jesper Dangaard Brouer wrote:
> >>>>
> >>>>> Based on patch from: Hans Schillstrom
> >>>>>
> >>>>> IPv6 headers must be processed in order of appearance,
> >>>>> neither can it be assumed that Upper layer headers is first.
> >>>>> If anything else than L4 is the first header IPVS will throw it.
> >>>>>
> >>>>> IPVS will write SNAT & DNAT modifications at a fixed pos which
> >>>>> will corrupt the message. Proper header position must be found
> >>>>> before writing modifying packet.
> >>>>>
> >>>>> This patch contains a lot of API changes. This is done, to avoid
> >>>>> the costly scan of finding the IPv6 headers, via ipv6_find_hdr().
> >>>>> Finding the IPv6 headers is done as early as possible, and passed
> >>>>> on as a pointer "struct ip_vs_iphdr *" to the affected functions.
> >>>>
> >>>> How about we change netfilter to set up the skb's transport header
> >>>> at an early time so we can avoid all (most of) these header scans
> >>>> in netfilter?
> >>>
> >>> I think that would be great, maybe it should be global i.e. not only a netfilter issue.
> >>
> >> I think in most other cases the headers are supposed to be processed
> >> sequentially. One problem though - to be useful for netfilter/IPVS
> >> we'd also need to store the transport layer protocol somewhere.
> >
> > I guess that's the problem, adding it to the skb will not be popular ....
> > Right now I don't have a good solution, maybe a more generic netfilter ptr in the skb ...
>
> I guess inet6_skb_parm will be at least slightly more popular than
> adding it to the skb itself. The netfilter pointers are all used for
> optional things, so we can't really add it to any of those.
Okay, but how do we go from here?
Hans, should this hold back the patch ("ipvs: Fix faulty IPv6 extension
header handling in IPVS"). Or should we pursue our patch, and circle
back later once e.g. Patrick have found a generic solution for IPv6
transport header handling?
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH net-next V3 0/2] Add rtnl_link_ops support to IPoIB
From: Or Gerlitz @ 2012-08-29 9:41 UTC (permalink / raw)
To: davem, roland; +Cc: netdev
In-Reply-To: <1345724119-32110-1-git-send-email-ogerlitz@mellanox.com>
On 23/08/2012 15:15, Or Gerlitz wrote:
> This is about adding rtnl_link_ops to IPoIB, primarly addressing feedback from Dave on a similar patch that was part of the eIPoIB submission.
Dave, Roland
Any comment on this? The patch was originally submitted through netdev
so I kept the fixed version to go through that path as well. If this
makes things easier, we can merge that through net-next.
IPoIB has more proprietary sysfs entries, such as for setting the mode
(datagram or connected) -- in 2nd thought, non legacy ipoib child
interfaces created through rtnl shouldn't preserve these prop.
mechanisms and use rtnl for all sorts of configs, as done for
macvlan/8021q etc devices, correct? if this is indeed the case, I will
fix that and submit V3.
Or.
>
> Also added the releated iproute2 patch, for the sake of the review and
> testing, e.g example usages:
>
> $ ip link add link ib0 name ib0.1 type ipoib
> $ ip link add link ib0 name ib0.8001 type ipoib pkey 0x8001
>
> and the obvious
>
> $ link delete dev ib0.8001
> $ link delete dev ib0.1
>
> Changes from V2:
> - removed the notion of user defined index per child, since we can do well w.o it
> - for that end, make (an internal to ipoib) distrinction between legacy childs created
> through the old sysfs way to childs created using rtnl link ops
>
> Changes from V1:
> - applied feedback from Dave Miller to avoid using sysfs
> - added rtnl_link_ops support in ipoib and use them to add/delete childs
>
> Or Gerlitz (1):
> IB/ipoib: Add rtnl_link_ops support
>
> Documentation/infiniband/ipoib.txt | 3 +
> drivers/infiniband/ulp/ipoib/Makefile | 3 +-
> drivers/infiniband/ulp/ipoib/ipoib.h | 13 +++
> drivers/infiniband/ulp/ipoib/ipoib_main.c | 25 ++++-
> drivers/infiniband/ulp/ipoib/ipoib_netlink.c | 122 ++++++++++++++++++++++++++
> drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 102 ++++++++++++----------
> 6 files changed, 217 insertions(+), 51 deletions(-)
> create mode 100644 drivers/infiniband/ulp/ipoib/ipoib_netlink.c
>
^ permalink raw reply
* Re[2]: Re[3]: [PATCH 2/3] ipvs: Fix faulty IPv6 extension header handling in IPVS
From: Hans Schillstrom @ 2012-08-29 9:47 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Patrick McHardy, netdev, lvs-devel, Julian Anastasov,
Simon Horman, Wensong Zhang, netfilter-devel
Hi,
>To Hans and Patrick,
>
>On Mon, 2012-08-27 at 14:02 +0200, Patrick McHardy wrote:
>> On Mon, 27 Aug 2012, Hans Schillstrom wrote:
>>
>> >>>>
>> >>>> On Mon, 20 Aug 2012, Jesper Dangaard Brouer wrote:
>> >>>>
>> >>>>> Based on patch from: Hans Schillstrom
>> >>>>>
>> >>>>> IPv6 headers must be processed in order of appearance,
>> >>>>> neither can it be assumed that Upper layer headers is first.
>> >>>>> If anything else than L4 is the first header IPVS will throw it.
>> >>>>>
>> >>>>> IPVS will write SNAT & DNAT modifications at a fixed pos which
>> >>>>> will corrupt the message. Proper header position must be found
>> >>>>> before writing modifying packet.
>> >>>>>
>> >>>>> This patch contains a lot of API changes. This is done, to avoid
>> >>>>> the costly scan of finding the IPv6 headers, via ipv6_find_hdr().
>> >>>>> Finding the IPv6 headers is done as early as possible, and passed
>> >>>>> on as a pointer "struct ip_vs_iphdr *" to the affected functions.
>> >>>>
>> >>>> How about we change netfilter to set up the skb's transport header
>> >>>> at an early time so we can avoid all (most of) these header scans
>> >>>> in netfilter?
>> >>>
>> >>> I think that would be great, maybe it should be global i.e. not only a netfilter issue.
>> >>
>> >> I think in most other cases the headers are supposed to be processed
>> >> sequentially. One problem though - to be useful for netfilter/IPVS
>> >> we'd also need to store the transport layer protocol somewhere.
>> >
>> > I guess that's the problem, adding it to the skb will not be popular ....
>> > Right now I don't have a good solution, maybe a more generic netfilter ptr in the skb ...
>>
>> I guess inet6_skb_parm will be at least slightly more popular than
>> adding it to the skb itself. The netfilter pointers are all used for
>> optional things, so we can't really add it to any of those.
>
>Okay, but how do we go from here?
>
>Hans, should this hold back the patch ("ipvs: Fix faulty IPv6 extension
>header handling in IPVS"). Or should we pursue our patch, and circle
>back later once e.g. Patrick have found a generic solution for IPv6
>transport header handling?
Should we give it a try to put it in inet6_skb_parm
and minimize what we put there ?
I think it could be worth it.
^ permalink raw reply
* Re: [PATCH 2/3] ipvs: Fix faulty IPv6 extension header handling in IPVS
From: Jesper Dangaard Brouer @ 2012-08-29 11:46 UTC (permalink / raw)
To: Patrick McHardy
Cc: Hans Schillstrom, Hans Schillstrom, netdev, lvs-devel,
Julian Anastasov, Simon Horman, netfilter-devel
In-Reply-To: <nhkcv8u.3c7ad6d61f90aef8e9467df88476311d@obelix.schillstrom.com>
To Patrick,
On Wed, 2012-08-29 at 11:47 +0200, Hans Schillstrom wrote:
> >
> >On Mon, 2012-08-27 at 14:02 +0200, Patrick McHardy wrote:
> >> On Mon, 27 Aug 2012, Hans Schillstrom wrote:
> >>
> >> >>>>
> >> >>>> On Mon, 20 Aug 2012, Jesper Dangaard Brouer wrote:
> >> >>>>
> >> >>>>> Based on patch from: Hans Schillstrom
> >> >>>>>
> >> >>>>> IPv6 headers must be processed in order of appearance,
> >> >>>>> neither can it be assumed that Upper layer headers is first.
> >> >>>>> If anything else than L4 is the first header IPVS will throw it.
> >> >>>>>
> >> >>>>> IPVS will write SNAT & DNAT modifications at a fixed pos which
> >> >>>>> will corrupt the message. Proper header position must be found
> >> >>>>> before writing modifying packet.
> >> >>>>>
> >> >>>>> This patch contains a lot of API changes. This is done, to avoid
> >> >>>>> the costly scan of finding the IPv6 headers, via ipv6_find_hdr().
> >> >>>>> Finding the IPv6 headers is done as early as possible, and passed
> >> >>>>> on as a pointer "struct ip_vs_iphdr *" to the affected functions.
> >> >>>>
> >> >>>> How about we change netfilter to set up the skb's transport header
> >> >>>> at an early time so we can avoid all (most of) these header scans
> >> >>>> in netfilter?
> >> >>>
> >> >>> I think that would be great, maybe it should be global i.e. not only a netfilter issue.
> >> >>
> >> >> I think in most other cases the headers are supposed to be processed
> >> >> sequentially. One problem though - to be useful for netfilter/IPVS
> >> >> we'd also need to store the transport layer protocol somewhere.
> >> >
> >> > I guess that's the problem, adding it to the skb will not be popular ....
> >> > Right now I don't have a good solution, maybe a more generic netfilter ptr in the skb ...
> >>
> >> I guess inet6_skb_parm will be at least slightly more popular than
> >> adding it to the skb itself. The netfilter pointers are all used for
> >> optional things, so we can't really add it to any of those.
> >
> >Okay, but how do we go from here?
> >
> >Hans, should this hold back the patch ("ipvs: Fix faulty IPv6 extension
> >header handling in IPVS"). Or should we pursue our patch, and circle
> >back later once e.g. Patrick have found a generic solution for IPv6
> >transport header handling?
>
> Should we give it a try to put it in inet6_skb_parm
> and minimize what we put there ?
> I think it could be worth it.
Okay, but then I do need some help and guidance, especially from
Patrick, think.
First of all, where in the netfilter code, should we update the new
fields in inet6_skb_parm?
^ permalink raw reply
* Re: [PATCH 02/19] Cleaning up the IPv6 MTU checking in the IPVS xmit code, by using a common helper function __mtu_check_toobig_v6().
From: Patrick McHardy @ 2012-08-29 12:24 UTC (permalink / raw)
To: Jesper Dangaard Brouer; +Cc: Pablo Neira Ayuso, Netfilter Developers, netdev
In-Reply-To: <Pine.LNX.4.64.1208290959010.32281@ask.diku.dk>
On Wed, 29 Aug 2012, Jesper Dangaard Brouer wrote:
>
> Just a little nitpick.
>
> The original title/subj was:
> "ipvs: IPv6 MTU checking cleanup and bugfix"
>
> And the curr/used title/subj were part of the commit text.
Sorry, must have happened while importing the patch, I noticed it
afterwards, but was already too late.
^ permalink raw reply
* Re: [PATCH 03/19] netfilter: nf_conntrack_ipv6: improve fragmentation handling
From: Patrick McHardy @ 2012-08-29 12:27 UTC (permalink / raw)
To: Jesper Dangaard Brouer; +Cc: Pablo Neira Ayuso, Netfilter Developers, netdev
In-Reply-To: <Pine.LNX.4.64.1208291007520.32281@ask.diku.dk>
On Wed, 29 Aug 2012, Jesper Dangaard Brouer wrote:
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>
> And some nitpicks below...
>
>> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
>> index 5b2d63e..a4f6263 100644
>> --- a/net/ipv6/ip6_output.c
>> +++ b/net/ipv6/ip6_output.c
>> @@ -493,7 +493,8 @@ int ip6_forward(struct sk_buff *skb)
>> if (mtu < IPV6_MIN_MTU)
>> mtu = IPV6_MIN_MTU;
>>
>> - if (skb->len > mtu && !skb_is_gso(skb)) {
>> + if ((!skb->local_df && skb->len > mtu && !skb_is_gso(skb)) ||
>
> You use (!skb->local_df) to invalidate the use of skb->len, instead of
> (!IP6CB(skb)->frag_max_size), (which is okay, because you set local_df
> later). Is there are reason this check is better?
Just that it's consistent with ip6_output and the regular local_df
handling. It saves one extra condition in ip6_output.
>> + (IP6CB(skb)->frag_max_size && IP6CB(skb)->frag_max_size > mtu)) {
>
> Eric Dumazet would probably nitpick and say, it can be reduced to:
> (IP6CB(skb)->frag_max_size > mtu)
> ;-)
True. I'll fix that once Pablo has pulled in the patches.
^ permalink raw reply
* Re: Re[3]: [PATCH 2/3] ipvs: Fix faulty IPv6 extension header handling in IPVS
From: Patrick McHardy @ 2012-08-29 12:28 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Hans Schillstrom, netdev, lvs-devel, Julian Anastasov,
Simon Horman, Wensong Zhang, netfilter-devel
In-Reply-To: <1346233034.3069.450.camel@localhost>
On Wed, 29 Aug 2012, Jesper Dangaard Brouer wrote:
> To Hans and Patrick,
>
> On Mon, 2012-08-27 at 14:02 +0200, Patrick McHardy wrote:
>> On Mon, 27 Aug 2012, Hans Schillstrom wrote:
>>
>>>>>>
>>>>>> On Mon, 20 Aug 2012, Jesper Dangaard Brouer wrote:
>>>>>>
>>>>>>> Based on patch from: Hans Schillstrom
>>>>>>>
>>>>>>> IPv6 headers must be processed in order of appearance,
>>>>>>> neither can it be assumed that Upper layer headers is first.
>>>>>>> If anything else than L4 is the first header IPVS will throw it.
>>>>>>>
>>>>>>> IPVS will write SNAT & DNAT modifications at a fixed pos which
>>>>>>> will corrupt the message. Proper header position must be found
>>>>>>> before writing modifying packet.
>>>>>>>
>>>>>>> This patch contains a lot of API changes. This is done, to avoid
>>>>>>> the costly scan of finding the IPv6 headers, via ipv6_find_hdr().
>>>>>>> Finding the IPv6 headers is done as early as possible, and passed
>>>>>>> on as a pointer "struct ip_vs_iphdr *" to the affected functions.
>>>>>>
>>>>>> How about we change netfilter to set up the skb's transport header
>>>>>> at an early time so we can avoid all (most of) these header scans
>>>>>> in netfilter?
>>>>>
>>>>> I think that would be great, maybe it should be global i.e. not only a netfilter issue.
>>>>
>>>> I think in most other cases the headers are supposed to be processed
>>>> sequentially. One problem though - to be useful for netfilter/IPVS
>>>> we'd also need to store the transport layer protocol somewhere.
>>>
>>> I guess that's the problem, adding it to the skb will not be popular ....
>>> Right now I don't have a good solution, maybe a more generic netfilter ptr in the skb ...
>>
>> I guess inet6_skb_parm will be at least slightly more popular than
>> adding it to the skb itself. The netfilter pointers are all used for
>> optional things, so we can't really add it to any of those.
>
> Okay, but how do we go from here?
>
> Hans, should this hold back the patch ("ipvs: Fix faulty IPv6 extension
> header handling in IPVS"). Or should we pursue our patch, and circle
> back later once e.g. Patrick have found a generic solution for IPv6
> transport header handling?
I don't think we can do much better than using inet6_skb_parm. I think
the main question is whether it is really worth it, the iteration
shouldn't be that expensive in most cases.
^ permalink raw reply
* Re: [PATCH 2/3] ipvs: Fix faulty IPv6 extension header handling in IPVS
From: Patrick McHardy @ 2012-08-29 12:34 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Hans Schillstrom, Hans Schillstrom, netdev, lvs-devel,
Julian Anastasov, Simon Horman, netfilter-devel
In-Reply-To: <1346240770.3069.477.camel@localhost>
On Wed, 29 Aug 2012, Jesper Dangaard Brouer wrote:
> To Patrick,
>
> On Wed, 2012-08-29 at 11:47 +0200, Hans Schillstrom wrote:
>>>
>>> On Mon, 2012-08-27 at 14:02 +0200, Patrick McHardy wrote:
>>>> On Mon, 27 Aug 2012, Hans Schillstrom wrote:
>>>>
>>>>>>>> How about we change netfilter to set up the skb's transport header
>>>>>>>> at an early time so we can avoid all (most of) these header scans
>>>>>>>> in netfilter?
>>>>>>>
>>>>>>> I think that would be great, maybe it should be global i.e. not only a netfilter issue.
>>>>>>
>>>>>> I think in most other cases the headers are supposed to be processed
>>>>>> sequentially. One problem though - to be useful for netfilter/IPVS
>>>>>> we'd also need to store the transport layer protocol somewhere.
>>>>>
>>>>> I guess that's the problem, adding it to the skb will not be popular ....
>>>>> Right now I don't have a good solution, maybe a more generic netfilter ptr in the skb ...
>>>>
>>>> I guess inet6_skb_parm will be at least slightly more popular than
>>>> adding it to the skb itself. The netfilter pointers are all used for
>>>> optional things, so we can't really add it to any of those.
>>>
>>> Okay, but how do we go from here?
>>>
>>> Hans, should this hold back the patch ("ipvs: Fix faulty IPv6 extension
>>> header handling in IPVS"). Or should we pursue our patch, and circle
>>> back later once e.g. Patrick have found a generic solution for IPv6
>>> transport header handling?
>>
>> Should we give it a try to put it in inet6_skb_parm
>> and minimize what we put there ?
>> I think it could be worth it.
>
> Okay, but then I do need some help and guidance, especially from
> Patrick, think.
>
> First of all, where in the netfilter code, should we update the new
> fields in inet6_skb_parm?
Good question. I think we'd need at least three spots since every one
of these subsystems can be used indepedently from each other:
- conntrack/IPVS: PRE_ROUTING/LOCAL_OUT at lowest priority
- ip6tables: first time packet hits ip6t_do_table()?
Actually, looking at ipv6_rcv(), this might not work at all since it
sets skb->transport_header to the first header following the IPv6
header. This is used when processing extension headers by IPv6.
^ permalink raw reply
* Re[2]: Re[3]: [PATCH 2/3] ipvs: Fix faulty IPv6 extension header handling in IPVS
From: Hans Schillstrom @ 2012-08-29 12:36 UTC (permalink / raw)
To: Patrick McHardy
Cc: Jesper Dangaard Brouer, netdev, lvs-devel, Julian Anastasov,
Simon Horman, Wensong Zhang, netfilter-devel
>
>On Wed, 29 Aug 2012, Jesper Dangaard Brouer wrote:
>
>> To Hans and Patrick,
>>
>> On Mon, 2012-08-27 at 14:02 +0200, Patrick McHardy wrote:
>>> On Mon, 27 Aug 2012, Hans Schillstrom wrote:
>>>
>>>>>>>
>>>>>>> On Mon, 20 Aug 2012, Jesper Dangaard Brouer wrote:
>>>>>>>
>>>>>>>> Based on patch from: Hans Schillstrom
>>>>>>>>
>>>>>>>> IPv6 headers must be processed in order of appearance,
>>>>>>>> neither can it be assumed that Upper layer headers is first.
>>>>>>>> If anything else than L4 is the first header IPVS will throw it.
>>>>>>>>
>>>>>>>> IPVS will write SNAT & DNAT modifications at a fixed pos which
>>>>>>>> will corrupt the message. Proper header position must be found
>>>>>>>> before writing modifying packet.
>>>>>>>>
>>>>>>>> This patch contains a lot of API changes. This is done, to avoid
>>>>>>>> the costly scan of finding the IPv6 headers, via ipv6_find_hdr().
>>>>>>>> Finding the IPv6 headers is done as early as possible, and passed
>>>>>>>> on as a pointer "struct ip_vs_iphdr *" to the affected functions.
>>>>>>>
>>>>>>> How about we change netfilter to set up the skb's transport header
>>>>>>> at an early time so we can avoid all (most of) these header scans
>>>>>>> in netfilter?
>>>>>>
>>>>>> I think that would be great, maybe it should be global i.e. not only a netfilter issue.
>>>>>
>>>>> I think in most other cases the headers are supposed to be processed
>>>>> sequentially. One problem though - to be useful for netfilter/IPVS
>>>>> we'd also need to store the transport layer protocol somewhere.
>>>>
>>>> I guess that's the problem, adding it to the skb will not be popular ....
>>>> Right now I don't have a good solution, maybe a more generic netfilter ptr in the skb ...
>>>
>>> I guess inet6_skb_parm will be at least slightly more popular than
>>> adding it to the skb itself. The netfilter pointers are all used for
>>> optional things, so we can't really add it to any of those.
>>
>> Okay, but how do we go from here?
>>
>> Hans, should this hold back the patch ("ipvs: Fix faulty IPv6 extension
>> header handling in IPVS"). Or should we pursue our patch, and circle
>> back later once e.g. Patrick have found a generic solution for IPv6
>> transport header handling?
>
>I don't think we can do much better than using inet6_skb_parm. I think
>the main question is whether it is really worth it, the iteration
>shouldn't be that expensive in most cases.
Well, if we start using it it could be worth it...
As a first sketch I think adding protocol and offset to inet6_skb_parm would be sufficient,
and then scan the header in ipv6_defrag() which is a quite early ...
/Hans
^ permalink raw reply
* Re: [PATCH net-next V3 0/2] Add rtnl_link_ops support to IPoIB
From: Patrick McHardy @ 2012-08-29 12:45 UTC (permalink / raw)
To: Or Gerlitz; +Cc: davem, roland, netdev
In-Reply-To: <503DE3AC.30701@mellanox.com>
On Wed, 29 Aug 2012, Or Gerlitz wrote:
> On 23/08/2012 15:15, Or Gerlitz wrote:
>> This is about adding rtnl_link_ops to IPoIB, primarly addressing feedback
>> from Dave on a similar patch that was part of the eIPoIB submission.
>
> Dave, Roland
>
> Any comment on this? The patch was originally submitted through netdev so I
> kept the fixed version to go through that path as well. If this makes things
> easier, we can merge that through net-next.
>
> IPoIB has more proprietary sysfs entries, such as for setting the mode
> (datagram or connected) -- in 2nd thought, non legacy ipoib child interfaces
> created through rtnl shouldn't preserve these prop. mechanisms and use rtnl
> for all sorts of configs, as done for macvlan/8021q etc devices, correct? if
> this is indeed the case, I will fix that and submit V3.
This is how rtnl_link was intended to be used, so you can set and query
all attributes in an atomic fashion and get notifications on changes.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox