Opened 8 years ago

Closed 8 years ago

#1534 closed defect (fixed)

socket creator should set IPV6_USE_MIN_MTU for AF_INET6 UDP sockets

Reported by: jinmei Owned by: vorner
Priority: medium Milestone: Sprint-20120306
Component: ~Boss of BIND (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 2 Add Hours to Ticket: 0
Total Hours: 4.05 Internal?: no

Description

Otherwise it will be possible that a large response (with EDNS0) gets
lost due to exceeding a link MTU at some intermediate router.

Explicitly disabling path MTU discovery for IPv4 may also be helpful
(setting IP_DONTFRAG to 0).

Subtickets

Change History (10)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 2

comment:2 Changed 8 years ago by jinmei

  • Milestone changed from Year 3 Task Backlog to Next-Sprint-Proposed

comment:3 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120221

comment:4 Changed 8 years ago by jinmei

  • Priority changed from critical to major

comment:5 Changed 8 years ago by vorner

  • Owner set to vorner
  • Status changed from new to accepted

comment:6 Changed 8 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from accepted to reviewing

Hello

It should be ready for review. As discussed on the mailing list, the test for MTU is disabled, as it returns something else than is being set. Also, Shane noted on jabber that the socket leaked whenever there was an error other than the socket() call failing, so I fixed that.

The changelog could be like this:

[bug]		vorner
The UDP IPv6 packets are now correctly fragmented for maximum guaranteed MTU,
so they won't get lost because being too large for some hop.

comment:7 Changed 8 years ago by jelte

  • Owner changed from UnAssigned to vorner

Not that it could currently be a problem I think, but it seems slightly better to me to only set IPV6_MTU if IPV6_USE_MIN_MTU actually does not exist; So I propose to change the code slightly to

if defined(IPV6_USE_MIN_MTU)
    foo
elif defined(IPV6_MTU)
    bar
fi

I would also like to request that you add a comment in the unittest file at closeCall(), warning that you should really reset the close_called value before you run the code that is supposed to call close :) (i'd almost suggest one would use a fixture but the hassles of bound methods are probably not worth working around for the current two tests)

Apart from that, it looks good :)

comment:8 Changed 8 years ago by vorner

  • Owner changed from vorner to jelte

OK, I did something in that direction.

comment:9 Changed 8 years ago by jelte

  • Owner changed from jelte to vorner

I suddenly encountered one additional linker problem, my makefile needs LDADD libexceptions. I've added it and pushed the change.

Changes look fine, and if the above is OK, this can be merged.

comment:10 Changed 8 years ago by vorner

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 4.05

Thank you, closing. (but your change was already in master)

Note: See TracTickets for help on using tickets.