DAHDI-Linux
  1. DAHDI-Linux
  2. DAHLIN-226

[patch] dahdi-base locks channel-exporting module in kernel if channel open fails

    Details

    • Type: Bug Bug
    • Status: Closed
    • Severity: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.0.1
    • Target Release Version/s: None
    • Component/s: dahdi (the module)
    • Labels:
      None
    • Mantis ID:
      18422
    • Regression:
      No

      Description

      If chan->span->ops->open() fails, then DAHDI_FLAGBIT_OPEN never
      gets set and module_put is never called on the exporter module.

                • ADDITIONAL INFORMATION ******

      Have not checked on 2.3.0, but the problem likely exists
      there too. I have checked 2.4.0 and the problem has not
      been fixed on that, neither (apparently nobody's channels
      ever fail to open() except mine .

        Activity

        Hide
        Shaun Ruffell added a comment -

        avarit: Thanks for opening the issue. I had to delete the code from the description since the project needs a contributors license agreement from contributors for all patches.
        https://issues.asterisk.org/view_license_agreement.php

        Do you want to attach a patch to the issue? I think there is enough information left in the description to act if you are not planning on doing that.

        Thanks again.

        Show
        Shaun Ruffell added a comment - avarit: Thanks for opening the issue. I had to delete the code from the description since the project needs a contributors license agreement from contributors for all patches. https://issues.asterisk.org/view_license_agreement.php Do you want to attach a patch to the issue? I think there is enough information left in the description to act if you are not planning on doing that. Thanks again.
        Hide
        Angelos Varvitsiotis added a comment -

        Hi sruffell,

        Thanks for responding and for taking appropriate action. My
        userid is avarvit, not avarit. I have now signed the license,
        so I guess I am allowed to (re)post the patch without anyone
        having to take it down for legal reasons. Here it is:

        (patch removed by sruffell)

        Show
        Angelos Varvitsiotis added a comment - Hi sruffell, Thanks for responding and for taking appropriate action. My userid is avarvit, not avarit. I have now signed the license, so I guess I am allowed to (re)post the patch without anyone having to take it down for legal reasons. Here it is: (patch removed by sruffell)
        Hide
        Shaun Ruffell added a comment -

        avarvit: sorry about misspelling your username.

        I did have to remove it again (sorry). All code submissions need to be updated via the "Upload File" block on the issue, and then select the "this is a code or documentation submission".

        You won't be able to upload until after your license submission has been reviewed which will most likely be on Monday.

        Show
        Shaun Ruffell added a comment - avarvit: sorry about misspelling your username. I did have to remove it again (sorry). All code submissions need to be updated via the "Upload File" block on the issue, and then select the "this is a code or documentation submission". You won't be able to upload until after your license submission has been reviewed which will most likely be on Monday.
        Hide
        Angelos Varvitsiotis added a comment -

        OK then, I guess the following fix instructions do not fall
        under any reasonable definition of "code", so you will not
        have to kill them (now three times in a row): the suggested fix
        is, around line 2714, check the return value from the open
        function; if that value is non-zero, then invoke module_put,
        because in that execution path module_put is not called
        anywhere else and thus the exporting module remains locked in
        the kernel.

        Show
        Angelos Varvitsiotis added a comment - OK then, I guess the following fix instructions do not fall under any reasonable definition of "code", so you will not have to kill them (now three times in a row): the suggested fix is, around line 2714, check the return value from the open function; if that value is non-zero, then invoke module_put, because in that execution path module_put is not called anywhere else and thus the exporting module remains locked in the kernel.
        Hide
        Shaun Ruffell added a comment -

        avarvit: I've attached a potential patch against the current trunk. Mind if I put your real name / email address in it?

        Actually, I just checked and it looks like your license has been approved. So you could upload your patch with the "Upload File" button.

        Not required, but if you use git to make your patch, then I can use (mostly) your commit message.

        But if you don't want to fool with it, I'll commit the patch that I just attached.

        Show
        Shaun Ruffell added a comment - avarvit: I've attached a potential patch against the current trunk. Mind if I put your real name / email address in it? Actually, I just checked and it looks like your license has been approved. So you could upload your patch with the "Upload File" button. Not required, but if you use git to make your patch, then I can use (mostly) your commit message. But if you don't want to fool with it, I'll commit the patch that I just attached.
        Hide
        Angelos Varvitsiotis added a comment -

        Hi sruffel,
        I took a look at your patch (without trying it) and it looks
        good. It seems that, in comparison to what I originally
        posted, you have just added a temp variable "ops" to hold
        the channel's span ops; other than that, the patch looks
        identical to my originally posted code.

        There is no need for me to submit a patch of "my own". Yes,
        it's fine if you put my real name and email in the patch
        you have produced. Thanks for taking immediate action on
        the issue!

        Show
        Angelos Varvitsiotis added a comment - Hi sruffel, I took a look at your patch (without trying it) and it looks good. It seems that, in comparison to what I originally posted, you have just added a temp variable "ops" to hold the channel's span ops; other than that, the patch looks identical to my originally posted code. There is no need for me to submit a patch of "my own". Yes, it's fine if you put my real name and email in the patch you have produced. Thanks for taking immediate action on the issue!
        Hide
        Digium Subversion added a comment -

        Repository: dahdi
        Revision: 9510

        U linux/trunk/drivers/dahdi/dahdi-base.c

        ------------------------------------------------------------------------
        r9510 | sruffell | 2010-12-07 08:20:39 -0600 (Tue, 07 Dec 2010) | 13 lines

        dahdi: Prevent unloadable module on failed open.

        If chan->span->ops->open() fails then the reference count of the module
        implementing the board driver will not be decremented. The result is a
        module that would always be "in use" and unloadable.

        This change makes sure to release that reference when open failed.

        (closes issue DAHLIN-226)
        Reported by: avarvit

        Signed-off-by: Shaun Ruffell <sruffell@digium.com>
        Acked-by: Angelos Varvitsiotis <avarvit@admin.grnet.gr>
        ------------------------------------------------------------------------

        http://svn.digium.com/view/dahdi?view=rev&revision=9510

        Show
        Digium Subversion added a comment - Repository: dahdi Revision: 9510 U linux/trunk/drivers/dahdi/dahdi-base.c ------------------------------------------------------------------------ r9510 | sruffell | 2010-12-07 08:20:39 -0600 (Tue, 07 Dec 2010) | 13 lines dahdi: Prevent unloadable module on failed open. If chan->span->ops->open() fails then the reference count of the module implementing the board driver will not be decremented. The result is a module that would always be "in use" and unloadable. This change makes sure to release that reference when open failed. (closes issue DAHLIN-226 ) Reported by: avarvit Signed-off-by: Shaun Ruffell <sruffell@digium.com> Acked-by: Angelos Varvitsiotis <avarvit@admin.grnet.gr> ------------------------------------------------------------------------ http://svn.digium.com/view/dahdi?view=rev&revision=9510
        Hide
        Digium Subversion added a comment -

        Repository: dahdi
        Revision: 9530

        U linux/trunk/drivers/dahdi/dahdi-base.c

        ------------------------------------------------------------------------
        r9530 | sruffell | 2010-12-13 08:57:13 -0600 (Mon, 13 Dec 2010) | 11 lines

        dahdi: Do not dereference chan->span for pseudo channels.

        Fixes a regression introduced in r9510 which saves a pointer to the ops
        member of a channel's span before checking if the channel is a pseudo.
        Psuedo channels do not have spans.

        (issue DAHLIN-227)
        (issue DAHLIN-226)
        Reported by: alecdavis

        Signed-off-by: Shaun Ruffell <sruffell@digium.com>
        ------------------------------------------------------------------------

        http://svn.digium.com/view/dahdi?view=rev&revision=9530

        Show
        Digium Subversion added a comment - Repository: dahdi Revision: 9530 U linux/trunk/drivers/dahdi/dahdi-base.c ------------------------------------------------------------------------ r9530 | sruffell | 2010-12-13 08:57:13 -0600 (Mon, 13 Dec 2010) | 11 lines dahdi: Do not dereference chan->span for pseudo channels. Fixes a regression introduced in r9510 which saves a pointer to the ops member of a channel's span before checking if the channel is a pseudo. Psuedo channels do not have spans. (issue DAHLIN-227 ) (issue DAHLIN-226 ) Reported by: alecdavis Signed-off-by: Shaun Ruffell <sruffell@digium.com> ------------------------------------------------------------------------ http://svn.digium.com/view/dahdi?view=rev&revision=9530
        Hide
        Digium Subversion added a comment -

        Repository: dahdi
        Revision: 9682

        U linux/branches/2.4/drivers/dahdi/dahdi-base.c

        ------------------------------------------------------------------------
        r9682 | sruffell | 2011-01-20 23:31:26 -0600 (Thu, 20 Jan 2011) | 15 lines

        dahdi: Prevent unloadable module on failed open.

        If chan->span->ops->open() fails then the reference count of the module
        implementing the board driver will not be decremented. The result is a
        module that would always be "in use" and unloadable.

        This change makes sure to release that reference when open failed.

        (closes issue DAHLIN-226)
        Reported by: avarvit

        Signed-off-by: Shaun Ruffell <sruffell@digium.com>
        Acked-by: Angelos Varvitsiotis <avarvit@admin.grnet.gr>

        Origin: http://svnview.digium.com/svn/dahdi?view=rev&rev=9510
        ------------------------------------------------------------------------

        http://svn.digium.com/view/dahdi?view=rev&revision=9682

        Show
        Digium Subversion added a comment - Repository: dahdi Revision: 9682 U linux/branches/2.4/drivers/dahdi/dahdi-base.c ------------------------------------------------------------------------ r9682 | sruffell | 2011-01-20 23:31:26 -0600 (Thu, 20 Jan 2011) | 15 lines dahdi: Prevent unloadable module on failed open. If chan->span->ops->open() fails then the reference count of the module implementing the board driver will not be decremented. The result is a module that would always be "in use" and unloadable. This change makes sure to release that reference when open failed. (closes issue DAHLIN-226 ) Reported by: avarvit Signed-off-by: Shaun Ruffell <sruffell@digium.com> Acked-by: Angelos Varvitsiotis <avarvit@admin.grnet.gr> Origin: http://svnview.digium.com/svn/dahdi?view=rev&rev=9510 ------------------------------------------------------------------------ http://svn.digium.com/view/dahdi?view=rev&revision=9682

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development